Moving logic out of closure in createMethod and into helper methods.

Reviewed in https://codereview.appspot.com/7375057/
diff --git a/apiclient/discovery.py b/apiclient/discovery.py
index 07d96bc..c31d65e 100644
--- a/apiclient/discovery.py
+++ b/apiclient/discovery.py
@@ -12,7 +12,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-"""Client for discovery based APIs
+"""Client for discovery based APIs.
 
 A client library for Google's discovery based APIs.
 """
@@ -27,6 +27,7 @@
 
 import copy
 import httplib2
+import keyword
 import logging
 import os
 import re
@@ -67,17 +68,29 @@
 URITEMPLATE = re.compile('{[^}]*}')
 VARNAME = re.compile('[a-zA-Z0-9_-]+')
 DISCOVERY_URI = ('https://www.googleapis.com/discovery/v1/apis/'
-  '{api}/{apiVersion}/rest')
+                 '{api}/{apiVersion}/rest')
 DEFAULT_METHOD_DOC = 'A description of how to use this function'
+HTTP_PAYLOAD_METHODS = frozenset(['PUT', 'POST', 'PATCH'])
+_MEDIA_SIZE_BIT_SHIFTS = {'KB': 10, 'MB': 20, 'GB': 30, 'TB': 40}
+BODY_PARAMETER_DEFAULT_VALUE = {
+    'description': 'The request body.',
+    'type': 'object',
+    'required': True,
+}
+MEDIA_BODY_PARAMETER_DEFAULT_VALUE = {
+  'description': ('The filename of the media request body, or an instance '
+                  'of a MediaUpload object.'),
+  'type': 'string',
+  'required': False,
+}
 
 # Parameters accepted by the stack, but not visible via discovery.
-STACK_QUERY_PARAMETERS = ['trace', 'pp', 'userip', 'strict']
+# TODO(dhermes): Remove 'userip' in 'v2'.
+STACK_QUERY_PARAMETERS = frozenset(['trace', 'pp', 'userip', 'strict'])
+STACK_QUERY_PARAMETER_DEFAULT_VALUE = {'type': 'string', 'location': 'query'}
 
-# Python reserved words.
-RESERVED_WORDS = ['and', 'assert', 'break', 'class', 'continue', 'def', 'del',
-                  'elif', 'else', 'except', 'exec', 'finally', 'for', 'from',
-                  'global', 'if', 'import', 'in', 'is', 'lambda', 'not', 'or',
-                  'pass', 'print', 'raise', 'return', 'try', 'while', 'body']
+# Library-specific reserved words beyond Python keywords.
+RESERVED_WORDS = frozenset(['body'])
 
 
 def fix_method_name(name):
@@ -89,7 +102,7 @@
   Returns:
     The name with a '_' prefixed if the name is a reserved word.
   """
-  if name in RESERVED_WORDS:
+  if keyword.iskeyword(name) or name in RESERVED_WORDS:
     return name + '_'
   else:
     return name
@@ -291,14 +304,6 @@
       return str(value)
 
 
-MULTIPLIERS = {
-    "KB": 2 ** 10,
-    "MB": 2 ** 20,
-    "GB": 2 ** 30,
-    "TB": 2 ** 40,
-    }
-
-
 def _media_size_to_long(maxSize):
   """Convert a string media size, such as 10GB or 3TB into an integer.
 
@@ -309,13 +314,166 @@
     The size as an integer value.
   """
   if len(maxSize) < 2:
-    return 0
+    return 0L
   units = maxSize[-2:].upper()
-  multiplier = MULTIPLIERS.get(units, 0)
-  if multiplier:
-    return int(maxSize[:-2]) * multiplier
+  bit_shift = _MEDIA_SIZE_BIT_SHIFTS.get(units)
+  if bit_shift is not None:
+    return long(maxSize[:-2]) << bit_shift
   else:
-    return int(maxSize)
+    return long(maxSize)
+
+
+def _media_path_url_from_info(root_desc, path_url):
+  """Creates an absolute media path URL.
+
+  Constructed using the API root URI and service path from the discovery
+  document and the relative path for the API method.
+
+  Args:
+    root_desc: Dictionary; the entire original deserialized discovery document.
+    path_url: String; the relative URL for the API method. Relative to the API
+        root, which is specified in the discovery document.
+
+  Returns:
+    String; the absolute URI for media upload for the API method.
+  """
+  return '%(root)supload/%(service_path)s%(path)s' % {
+      'root': root_desc['rootUrl'],
+      'service_path': root_desc['servicePath'],
+      'path': path_url,
+  }
+
+
+def _fix_up_parameters(method_desc, root_desc, http_method):
+  """Updates parameters of an API method with values specific to this library.
+
+  Specifically, adds whatever global parameters are specified by the API to the
+  parameters for the individual method. Also adds parameters which don't
+  appear in the discovery document, but are available to all discovery based
+  APIs (these are listed in STACK_QUERY_PARAMETERS).
+
+  SIDE EFFECTS: This updates the parameters dictionary object in the method
+  description.
+
+  Args:
+    method_desc: Dictionary with metadata describing an API method. Value comes
+        from the dictionary of methods stored in the 'methods' key in the
+        deserialized discovery document.
+    root_desc: Dictionary; the entire original deserialized discovery document.
+    http_method: String; the HTTP method used to call the API method described
+        in method_desc.
+
+  Returns:
+    The updated Dictionary stored in the 'parameters' key of the method
+        description dictionary.
+  """
+  parameters = method_desc.setdefault('parameters', {})
+
+  # Add in the parameters common to all methods.
+  for name, description in root_desc.get('parameters', {}).iteritems():
+    parameters[name] = description
+
+  # Add in undocumented query parameters.
+  for name in STACK_QUERY_PARAMETERS:
+    parameters[name] = STACK_QUERY_PARAMETER_DEFAULT_VALUE.copy()
+
+  # Add 'body' (our own reserved word) to parameters if the method supports
+  # a request payload.
+  if http_method in HTTP_PAYLOAD_METHODS and 'request' in method_desc:
+    body = BODY_PARAMETER_DEFAULT_VALUE.copy()
+    body.update(method_desc['request'])
+    parameters['body'] = body
+
+  return parameters
+
+
+def _fix_up_media_upload(method_desc, root_desc, path_url, parameters):
+  """Updates parameters of API by adding 'media_body' if supported by method.
+
+  SIDE EFFECTS: If the method supports media upload and has a required body,
+  sets body to be optional (required=False) instead. Also, if there is a
+  'mediaUpload' in the method description, adds 'media_upload' key to
+  parameters.
+
+  Args:
+    method_desc: Dictionary with metadata describing an API method. Value comes
+        from the dictionary of methods stored in the 'methods' key in the
+        deserialized discovery document.
+    root_desc: Dictionary; the entire original deserialized discovery document.
+    path_url: String; the relative URL for the API method. Relative to the API
+        root, which is specified in the discovery document.
+    parameters: A dictionary describing method parameters for method described
+        in method_desc.
+
+  Returns:
+    Triple (accept, max_size, media_path_url) where:
+      - accept is a list of strings representing what content types are
+        accepted for media upload. Defaults to empty list if not in the
+        discovery document.
+      - max_size is a long representing the max size in bytes allowed for a
+        media upload. Defaults to 0L if not in the discovery document.
+      - media_path_url is a String; the absolute URI for media upload for the
+        API method. Constructed using the API root URI and service path from
+        the discovery document and the relative path for the API method. If
+        media upload is not supported, this is None.
+  """
+  media_upload = method_desc.get('mediaUpload', {})
+  accept = media_upload.get('accept', [])
+  max_size = _media_size_to_long(media_upload.get('maxSize', ''))
+  media_path_url = None
+
+  if media_upload:
+    media_path_url = _media_path_url_from_info(root_desc, path_url)
+    parameters['media_body'] = MEDIA_BODY_PARAMETER_DEFAULT_VALUE.copy()
+    if 'body' in parameters:
+      parameters['body']['required'] = False
+
+  return accept, max_size, media_path_url
+
+
+def _fix_up_method_description(method_desc, root_desc):
+  """Updates a method description in a discovery document.
+
+  SIDE EFFECTS: Changes the parameters dictionary in the method description with
+  extra parameters which are used locally.
+
+  Args:
+    method_desc: Dictionary with metadata describing an API method. Value comes
+        from the dictionary of methods stored in the 'methods' key in the
+        deserialized discovery document.
+    root_desc: Dictionary; the entire original deserialized discovery document.
+
+  Returns:
+    Tuple (path_url, http_method, method_id, accept, max_size, media_path_url)
+    where:
+      - path_url is a String; the relative URL for the API method. Relative to
+        the API root, which is specified in the discovery document.
+      - http_method is a String; the HTTP method used to call the API method
+        described in the method description.
+      - method_id is a String; the name of the RPC method associated with the
+        API method, and is in the method description in the 'id' key.
+      - accept is a list of strings representing what content types are
+        accepted for media upload. Defaults to empty list if not in the
+        discovery document.
+      - max_size is a long representing the max size in bytes allowed for a
+        media upload. Defaults to 0L if not in the discovery document.
+      - media_path_url is a String; the absolute URI for media upload for the
+        API method. Constructed using the API root URI and service path from
+        the discovery document and the relative path for the API method. If
+        media upload is not supported, this is None.
+  """
+  path_url = method_desc['path']
+  http_method = method_desc['httpMethod']
+  method_id = method_desc['id']
+
+  parameters = _fix_up_parameters(method_desc, root_desc, http_method)
+  # Order is important. `_fix_up_media_upload` needs `method_desc` to have a
+  # 'parameters' key and needs to know if there is a 'body' parameter because it
+  # also sets a 'media_body' parameter.
+  accept, max_size, media_path_url = _fix_up_media_upload(
+      method_desc, root_desc, path_url, parameters)
+
+  return path_url, http_method, method_id, accept, max_size, media_path_url
 
 
 def createMethod(methodName, methodDesc, rootDesc, schema):
@@ -329,54 +487,8 @@
     schema: object, mapping of schema names to schema descriptions.
   """
   methodName = fix_method_name(methodName)
-  pathUrl = methodDesc['path']
-  httpMethod = methodDesc['httpMethod']
-  methodId = methodDesc['id']
-
-  mediaPathUrl = None
-  accept = []
-  maxSize = 0
-  if 'mediaUpload' in methodDesc:
-    mediaUpload = methodDesc['mediaUpload']
-    mediaPathUrl = (rootDesc['rootUrl'] + 'upload/' + rootDesc['servicePath']
-                    + pathUrl)
-    accept = mediaUpload['accept']
-    maxSize = _media_size_to_long(mediaUpload.get('maxSize', ''))
-
-  if 'parameters' not in methodDesc:
-    methodDesc['parameters'] = {}
-
-  # Add in the parameters common to all methods.
-  for name, desc in rootDesc.get('parameters', {}).iteritems():
-    methodDesc['parameters'][name] = desc
-
-  # Add in undocumented query parameters.
-  for name in STACK_QUERY_PARAMETERS:
-    methodDesc['parameters'][name] = {
-        'type': 'string',
-        'location': 'query'
-        }
-
-  if httpMethod in ['PUT', 'POST', 'PATCH'] and 'request' in methodDesc:
-    methodDesc['parameters']['body'] = {
-        'description': 'The request body.',
-        'type': 'object',
-        'required': True,
-        }
-    if 'request' in methodDesc:
-      methodDesc['parameters']['body'].update(methodDesc['request'])
-    else:
-      methodDesc['parameters']['body']['type'] = 'object'
-  if 'mediaUpload' in methodDesc:
-    methodDesc['parameters']['media_body'] = {
-        'description':
-          'The filename of the media request body, or an instance of a '
-          'MediaUpload object.',
-        'type': 'string',
-        'required': False,
-        }
-    if 'body' in methodDesc['parameters']:
-      methodDesc['parameters']['body']['required'] = False
+  (pathUrl, httpMethod, methodId, accept,
+   maxSize, mediaPathUrl) = _fix_up_method_description(methodDesc, rootDesc)
 
   argmap = {} # Map from method parameter name to query parameter name
   required_params = [] # Required parameters
diff --git a/tests/test_discovery.py b/tests/test_discovery.py
index 46d8e33..3e19e2a 100644
--- a/tests/test_discovery.py
+++ b/tests/test_discovery.py
@@ -23,6 +23,7 @@
 
 __author__ = 'jcgregorio@google.com (Joe Gregorio)'
 
+import copy
 import datetime
 import gflags
 import httplib2
@@ -35,16 +36,22 @@
 
 
 try:
-    from urlparse import parse_qs
+  from urlparse import parse_qs
 except ImportError:
-    from cgi import parse_qs
+  from cgi import parse_qs
 
 
 from apiclient.discovery import _add_query_parameter
+from apiclient.discovery import _fix_up_media_upload
+from apiclient.discovery import _fix_up_method_description
+from apiclient.discovery import _fix_up_parameters
 from apiclient.discovery import build
 from apiclient.discovery import build_from_document
 from apiclient.discovery import DISCOVERY_URI
 from apiclient.discovery import key2param
+from apiclient.discovery import MEDIA_BODY_PARAMETER_DEFAULT_VALUE
+from apiclient.discovery import STACK_QUERY_PARAMETERS
+from apiclient.discovery import STACK_QUERY_PARAMETER_DEFAULT_VALUE
 from apiclient.errors import HttpError
 from apiclient.errors import InvalidJsonError
 from apiclient.errors import MediaUploadSizeError
@@ -91,16 +98,176 @@
 
 
 class SetupHttplib2(unittest.TestCase):
+
   def test_retries(self):
     # Merely loading apiclient.discovery should set the RETRIES to 1.
     self.assertEqual(1, httplib2.RETRIES)
 
 
 class Utilities(unittest.TestCase):
+
+  def setUp(self):
+    with open(datafile('zoo.json'), 'r') as fh:
+      self.zoo_root_desc = simplejson.loads(fh.read())
+    self.zoo_get_method_desc = self.zoo_root_desc['methods']['query']
+    zoo_animals_resource = self.zoo_root_desc['resources']['animals']
+    self.zoo_insert_method_desc = zoo_animals_resource['methods']['insert']
+
   def test_key2param(self):
     self.assertEqual('max_results', key2param('max-results'))
     self.assertEqual('x007_bond', key2param('007-bond'))
 
+  def _base_fix_up_parameters_test(self, method_desc, http_method, root_desc):
+    self.assertEqual(method_desc['httpMethod'], http_method)
+
+    method_desc_copy = copy.deepcopy(method_desc)
+    self.assertEqual(method_desc, method_desc_copy)
+
+    parameters = _fix_up_parameters(method_desc_copy, root_desc, http_method)
+
+    self.assertNotEqual(method_desc, method_desc_copy)
+
+    for param_name in STACK_QUERY_PARAMETERS:
+      self.assertEqual(STACK_QUERY_PARAMETER_DEFAULT_VALUE,
+                       parameters[param_name])
+
+    for param_name, value in root_desc.get('parameters', {}).iteritems():
+      self.assertEqual(value, parameters[param_name])
+
+    return parameters
+
+  def test_fix_up_parameters_get(self):
+    parameters = self._base_fix_up_parameters_test(self.zoo_get_method_desc,
+                                                   'GET', self.zoo_root_desc)
+    # Since http_method is 'GET'
+    self.assertFalse(parameters.has_key('body'))
+
+  def test_fix_up_parameters_insert(self):
+    parameters = self._base_fix_up_parameters_test(self.zoo_insert_method_desc,
+                                                   'POST', self.zoo_root_desc)
+    body = {
+        'description': 'The request body.',
+        'type': 'object',
+        'required': True,
+        '$ref': 'Animal',
+    }
+    self.assertEqual(parameters['body'], body)
+
+  def test_fix_up_parameters_check_body(self):
+    dummy_root_desc = {}
+    no_payload_http_method = 'DELETE'
+    with_payload_http_method = 'PUT'
+
+    invalid_method_desc = {'response': 'Who cares'}
+    valid_method_desc = {'request': {'key1': 'value1', 'key2': 'value2'}}
+
+    parameters = _fix_up_parameters(invalid_method_desc, dummy_root_desc,
+                                    no_payload_http_method)
+    self.assertFalse(parameters.has_key('body'))
+
+    parameters = _fix_up_parameters(valid_method_desc, dummy_root_desc,
+                                    no_payload_http_method)
+    self.assertFalse(parameters.has_key('body'))
+
+    parameters = _fix_up_parameters(invalid_method_desc, dummy_root_desc,
+                                    with_payload_http_method)
+    self.assertFalse(parameters.has_key('body'))
+
+    parameters = _fix_up_parameters(valid_method_desc, dummy_root_desc,
+                                    with_payload_http_method)
+    body = {
+        'description': 'The request body.',
+        'type': 'object',
+        'required': True,
+        'key1': 'value1',
+        'key2': 'value2',
+    }
+    self.assertEqual(parameters['body'], body)
+
+  def _base_fix_up_method_description_test(
+      self, method_desc, initial_parameters, final_parameters,
+      final_accept, final_max_size, final_media_path_url):
+    fake_root_desc = {'rootUrl': 'http://root/',
+                      'servicePath': 'fake/'}
+    fake_path_url = 'fake-path/'
+
+    accept, max_size, media_path_url = _fix_up_media_upload(
+        method_desc, fake_root_desc, fake_path_url, initial_parameters)
+    self.assertEqual(accept, final_accept)
+    self.assertEqual(max_size, final_max_size)
+    self.assertEqual(media_path_url, final_media_path_url)
+    self.assertEqual(initial_parameters, final_parameters)
+
+  def test_fix_up_media_upload_no_initial_invalid(self):
+    invalid_method_desc = {'response': 'Who cares'}
+    self._base_fix_up_method_description_test(invalid_method_desc, {}, {},
+                                              [], 0, None)
+
+  def test_fix_up_media_upload_no_initial_valid_minimal(self):
+    valid_method_desc = {'mediaUpload': {'accept': []}}
+    final_parameters = {'media_body': MEDIA_BODY_PARAMETER_DEFAULT_VALUE}
+    self._base_fix_up_method_description_test(
+        valid_method_desc, {}, final_parameters, [], 0,
+        'http://root/upload/fake/fake-path/')
+
+  def test_fix_up_media_upload_no_initial_valid_full(self):
+    valid_method_desc = {'mediaUpload': {'accept': ['*/*'], 'maxSize': '10GB'}}
+    final_parameters = {'media_body': MEDIA_BODY_PARAMETER_DEFAULT_VALUE}
+    ten_gb = 10 * 2**30
+    self._base_fix_up_method_description_test(
+        valid_method_desc, {}, final_parameters, ['*/*'],
+        ten_gb, 'http://root/upload/fake/fake-path/')
+
+  def test_fix_up_media_upload_with_initial_invalid(self):
+    invalid_method_desc = {'response': 'Who cares'}
+    initial_parameters = {'body': {}}
+    self._base_fix_up_method_description_test(
+        invalid_method_desc, initial_parameters,
+        initial_parameters, [], 0, None)
+
+  def test_fix_up_media_upload_with_initial_valid_minimal(self):
+    valid_method_desc = {'mediaUpload': {'accept': []}}
+    initial_parameters = {'body': {}}
+    final_parameters = {'body': {'required': False},
+                        'media_body': MEDIA_BODY_PARAMETER_DEFAULT_VALUE}
+    self._base_fix_up_method_description_test(
+        valid_method_desc, initial_parameters, final_parameters, [], 0,
+        'http://root/upload/fake/fake-path/')
+
+  def test_fix_up_media_upload_with_initial_valid_full(self):
+    valid_method_desc = {'mediaUpload': {'accept': ['*/*'], 'maxSize': '10GB'}}
+    initial_parameters = {'body': {}}
+    final_parameters = {'body': {'required': False},
+                        'media_body': MEDIA_BODY_PARAMETER_DEFAULT_VALUE}
+    ten_gb = 10 * 2**30
+    self._base_fix_up_method_description_test(
+        valid_method_desc, initial_parameters, final_parameters, ['*/*'],
+        ten_gb, 'http://root/upload/fake/fake-path/')
+
+  def test_fix_up_method_description_get(self):
+    result = _fix_up_method_description(self.zoo_get_method_desc,
+                                        self.zoo_root_desc)
+    path_url = 'query'
+    http_method = 'GET'
+    method_id = 'bigquery.query'
+    accept = []
+    max_size = 0L
+    media_path_url = None
+    self.assertEqual(result, (path_url, http_method, method_id, accept,
+                              max_size, media_path_url))
+
+  def test_fix_up_method_description_insert(self):
+    result = _fix_up_method_description(self.zoo_insert_method_desc,
+                                        self.zoo_root_desc)
+    path_url = 'animals'
+    http_method = 'POST'
+    method_id = 'zoo.animals.insert'
+    accept = ['image/png']
+    max_size = 1024L
+    media_path_url = 'https://www.googleapis.com/upload/zoo/v1/animals'
+    self.assertEqual(result, (path_url, http_method, method_id, accept,
+                              max_size, media_path_url))
+
 
 class DiscoveryErrors(unittest.TestCase):