Move conversion of login_customer_id to a str to config module (googleads#136)

BenRKarl · web-flow · commit afbfda773193 · 2019-06-19T18:38:10.000-04:00
diff --git a/ChangeLog b/ChangeLog
@@ -1,3 +1,6 @@
+* 2.4.1:
+- Fix bug preventing login_customer_id to be loaded as an int
+
 * 2.4.0:
 - Add utf-8 encoding declaration in generated proto files
 - Add Service Account support
diff --git a/google/ads/google_ads/__init__.py b/google/ads/google_ads/__init__.py
@@ -21,4 +21,4 @@
 import google.ads.google_ads.util
 
 
-VERSION = '2.4.0'
+VERSION = '2.4.1'
diff --git a/google/ads/google_ads/client.py b/google/ads/google_ads/client.py
@@ -55,14 +55,10 @@ def _get_client_kwargs(cls, config_data):
         Raises:
             ValueError: If the configuration lacks a required field.
         """
-        login_customer_id = config_data.get('login_customer_id')
-        login_customer_id = str(
-                login_customer_id) if login_customer_id else None
-
         return {'credentials': oauth2.get_credentials(config_data),
                 'developer_token': config_data.get('developer_token'),
                 'endpoint': config_data.get('endpoint'),
-                'login_customer_id': login_customer_id,
+                'login_customer_id': config_data.get('login_customer_id'),
                 'logging_config': config_data.get('logging')}
 
     @classmethod
diff --git a/google/ads/google_ads/config.py b/google/ads/google_ads/config.py
@@ -47,6 +47,22 @@ def validation_wrapper(*args, **kwargs):
     return validation_wrapper
 
 
+def _config_parser_decorator(func):
+    """A decorator used to easily parse config values.
+
+    Since configs can be loaded from different locations such as env vars or
+    from YAML files it's possible that they may have inconsistent types that
+    need to be parsed to a different type. Add this decorator to any method
+    that returns the config as a dict.
+    """
+    @functools.wraps(func)
+    def parser_wrapper(*args, **kwargs):
+        config_dict = func(*args, **kwargs)
+        parsed_config = convert_login_customer_id_to_str(config_dict)
+        return parsed_config
+    return parser_wrapper
+
+
 def validate_dict(config_data):
     """Validates the given configuration dict.
 
@@ -87,6 +103,7 @@ def validate_login_customer_id(login_customer_id):
 
 
 @_config_validation_decorator
+@_config_parser_decorator
 def load_from_yaml_file(path=None):
     """Loads configuration data from a YAML file and returns it as a dict.
 
@@ -114,6 +131,7 @@ def load_from_yaml_file(path=None):
 
 
 @_config_validation_decorator
+@_config_parser_decorator
 def parse_yaml_document_to_dict(yaml_doc):
     """Parses a YAML document to a dict.
 
@@ -131,6 +149,7 @@ def parse_yaml_document_to_dict(yaml_doc):
 
 
 @_config_validation_decorator
+@_config_parser_decorator
 def load_from_env():
     """Loads configuration data from the environment and returns it as a dict.
 
@@ -171,3 +190,24 @@ def get_oauth2_service_account_keys():
         A tuple containing the required keys as strs.
     """
     return _OAUTH2_SERVICE_ACCOUNT_KEYS
+
+
+def convert_login_customer_id_to_str(config_data):
+    """Parses a config dict's login_customer_id attr value to a str.
+
+    Like many values from YAML it's possible for login_customer_id to
+    either be a str or an int. Since we actually run validations on this
+    value before making requests it's important to parse it to a str.
+
+    Args:
+        config_data: A config dict object.
+
+    Returns:
+        The same config dict object with a mutated login_customer_id attr.
+    """
+    login_customer_id = config_data.get('login_customer_id')
+
+    if login_customer_id:
+        config_data['login_customer_id'] = str(login_customer_id)
+
+    return config_data
diff --git a/setup.py b/setup.py
@@ -35,7 +35,7 @@
 
 setup(
     name='google-ads',
-    version='2.4.0',
+    version='2.4.1',
     author='Google LLC',
     author_email='googleapis-packages@google.com',
     classifiers=[
diff --git a/tests/client_test.py b/tests/client_test.py
@@ -232,6 +232,40 @@ def test_load_from_storage(self):
                 login_customer_id=None,
                 logging_config=None)
 
+    def test_load_from_storage_login_cid_int(self):
+        login_cid = 1234567890
+        config = {
+            'developer_token': self.developer_token,
+            'client_id': self.client_id,
+            'client_secret': self.client_secret,
+            'refresh_token': self.refresh_token,
+            'login_customer_id': login_cid}
+
+        file_path = os.path.join(os.path.expanduser('~'), 'google-ads.yaml')
+        self.fs.create_file(file_path, contents=yaml.safe_dump(config))
+        mock_credentials_instance = mock.Mock()
+
+        with mock.patch.object(
+            Client.GoogleAdsClient,
+            '__init__',
+            return_value=None
+        ) as mock_client_init, mock.patch.object(
+            Client.oauth2,
+            'get_installed_app_credentials',
+            return_value=mock_credentials_instance
+        ) as mock_credentials:
+            Client.GoogleAdsClient.load_from_storage()
+            mock_credentials.assert_called_once_with(
+                config.get('client_id'),
+                config.get('client_secret'),
+                config.get('refresh_token'))
+            mock_client_init.assert_called_once_with(
+                credentials=mock_credentials_instance,
+                developer_token=self.developer_token,
+                endpoint=None,
+                login_customer_id=str(login_cid),
+                logging_config=None)
+
     def test_load_from_storage_custom_path(self):
         config = {
             'developer_token': self.developer_token,
diff --git a/tests/config_test.py b/tests/config_test.py
@@ -75,6 +75,23 @@ def test_load_from_yaml_file_with_path(self):
         self.assertEqual(result['client_secret'], self.client_secret)
         self.assertEqual(result['refresh_token'], self.refresh_token)
 
+    def test_load_from_yaml_file_login_cid_int(self):
+        login_cid_int = 1234567890
+        file_path = os.path.join(os.path.expanduser('~'), 'google-ads.yaml')
+        self.fs.create_file(file_path, contents=yaml.safe_dump({
+            'login_customer_id': login_cid_int,
+            'developer_token': self.developer_token,
+            'client_id': self.client_id,
+            'client_secret': self.client_secret,
+            'refresh_token': self.refresh_token}))
+
+        result = config.load_from_yaml_file()
+
+        self.assertEqual(result['developer_token'], self.developer_token)
+        self.assertEqual(result['client_id'], self.client_id)
+        self.assertEqual(result['client_secret'], self.client_secret)
+        self.assertEqual(result['refresh_token'], self.refresh_token)
+
     def test_parse_yaml_document_to_dict(self):
         yaml_doc = ('client_id: {}\n'
                     'client_secret: {}\n'
@@ -199,3 +216,19 @@ def test_get_oauth2_installed_app_keys(self):
     def test_get_oauth2_service_account_keys(self):
         self.assertEqual(config.get_oauth2_service_account_keys(),
                          config._OAUTH2_SERVICE_ACCOUNT_KEYS)
+
+    def test_convert_login_customer_id_to_str_with_int(self):
+        config_data = {'login_customer_id': 1234567890}
+        expected = {'login_customer_id': '1234567890'}
+        self.assertEqual(config.convert_login_customer_id_to_str(config_data),
+                         expected)
+
+    def test_parse_login_customer_id_with_str(self):
+        config_data = {'login_customer_id': '1234567890'}
+        self.assertEqual(config.convert_login_customer_id_to_str(config_data),
+                         config_data)
+
+    def test_parse_login_customer_id_with_none(self):
+        config_data = {'not_login_customer_id': 1234567890}
+        self.assertEqual(config.convert_login_customer_id_to_str(config_data),
+                         config_data)

Original file line number	Diff line number	Diff line change
`@@ -21,4 +21,4 @@`
`21`	`21`	`import google.ads.google_ads.util`
`22`	`22`
`23`	`23`
`24`		`-VERSION = '2.4.0'`
	`24`	`+VERSION = '2.4.1'`