api_config: avoid duplicating initialization state logic

Currently the api_config.py is used as both a module containing methods
as well as defining a class that, until recently, was used to obtain
values from the class attributes instead of some instance state.

To avoid global state and reduce logic duplication, specifically
read_key(), move the module methods to class methods.  The ApiConfig
should be convenient on instantiation and should try to set some sane
initial state.

While we're at it reduce file stat(3) reading when calling
get_config_from_kwargs().  Return default api config instead of creating
new objects for every call an authorized session instance makes.

Signed-off-by: Jamie Couture <jamie.couture@nasdaq.com>

Author: couture-ql

nasdaqdatalink/__init__.py

@@ -1,6 +1,6 @@
 # -*- coding: utf-8 -*-
 
-from .api_config import ApiConfig, save_key, read_key
+from .api_config import ApiConfig, get_config_from_kwargs
 
 from .errors.data_link_error import *
 
@@ -16,6 +16,3 @@
 from .export_table import export_table
 from .get_table import get_table
 from .get_point_in_time import get_point_in_time
-
-
-read_key()

nasdaqdatalink/api_config.py

@@ -17,111 +17,96 @@ class ApiConfig:
     retry_status_codes = [429] + list(range(500, 512))
     verify_ssl = True
 
-    def read_key(self, filename=None):
-        if not os.path.isfile(filename):
-            raise_empty_file(filename)
-
-        with open(filename, 'r') as f:
-            apikey = get_first_non_empty(f)
-
-        if not apikey:
-            raise_empty_file(filename)
-
+    def __init__(self):
+        self.read_key()
+
+    def create_file(self, config_filename):
+        # Create the file as well as the parent dir if needed.
+        dirname = os.path.split(config_filename)[0]
+        if not os.path.isdir(dirname):
+            os.makedirs(dirname)
+        with os.fdopen(os.open(config_filename,
+                               os.O_WRONLY | os.O_CREAT, 0o600), 'w'):
+            pass
+
+    def create_file_if_necessary(self, config_filename):
+        if not os.path.isfile(config_filename):
+            self.create_file(config_filename)
+
+    def default_config_filename(self):
+        config_file = os.path.join('~', '.nasdaq', 'data_link_apikey')
+        return os.path.expanduser(config_file)
+
+    def config_file_exists(self, filename=None):
+        if filename is None:
+            filename = self.default_config_filename()
+
+        return os.path.isfile(filename)
+
+    def save_key(self, apikey, filename=None):
+        if filename is None:
+            filename = self.default_config_filename()
+            self.create_file_if_necessary(filename)
+
+        fileptr = open(filename, 'w')
+        fileptr.write(apikey)
+        fileptr.close()
         self.api_key = apikey
 
+    def raise_empty_file(self, config_filename):
+        raise ValueError("File '{:s}' is empty.".format(config_filename))
 
-def create_file(config_filename):
-    # Create the file as well as the parent dir if needed.
-    dirname = os.path.split(config_filename)[0]
-    if not os.path.isdir(dirname):
-        os.makedirs(dirname)
-    with os.fdopen(os.open(config_filename,
-                           os.O_WRONLY | os.O_CREAT, 0o600), 'w'):
-        pass
-
-
-def create_file_if_necessary(config_filename):
-    if not os.path.isfile(config_filename):
-        create_file(config_filename)
-
-
-def default_config_filename():
-    config_file = os.path.join('~', '.nasdaq', 'data_link_apikey')
-    return os.path.expanduser(config_file)
-
-
-def config_file_exists(filename=None):
-    if filename is None:
-        filename = default_config_filename()
-
-    return os.path.isfile(filename)
-
-
-def save_key(apikey, filename=None):
-    if filename is None:
-        filename = default_config_filename()
-        create_file_if_necessary(filename)
-
-    fileptr = open(filename, 'w')
-    fileptr.write(apikey)
-    fileptr.close()
-    ApiConfig.api_key = apikey
-
-
-def raise_empty_file(config_filename):
-    raise ValueError("File '{:s}' is empty.".format(config_filename))
+    def raise_empty_environment_variable(self):
+        raise ValueError("NASDAQ_DATA_LINK_API_KEY cannot be empty")
 
+    def get_first_non_empty(self, file_handle):
+        lines = [line.strip() for line in file_handle.readlines()]
+        return next((line for line in lines if line), None)
 
-def raise_empty_environment_variable():
-    raise ValueError("NASDAQ_DATA_LINK_API_KEY cannot be empty")
+    def read_key_from_file(self, filename=None):
+        if filename is None:
+            filename = self.default_config_filename()
 
+        if not os.path.isfile(filename):
+            self.raise_empty_file(filename)
 
-def get_first_non_empty(file_handle):
-    lines = [line.strip() for line in file_handle.readlines()]
-    return next((line for line in lines if line), None)
-
-
-def read_key_from_file(filename=None):
-    if filename is None:
-        filename = default_config_filename()
-
-    if not os.path.isfile(filename):
-        raise_empty_file(filename)
-
-    with open(filename, 'r') as f:
-        apikey = get_first_non_empty(f)
-
-    if not apikey:
-        raise_empty_file(filename)
-
-    ApiConfig.api_key = apikey
-
+        with open(filename, 'r') as f:
+            apikey = self.get_first_non_empty(f)
 
-def api_key_environment_variable_exists():
-    return NASDAQ_DATA_LINK_API_KEY in os.environ
+        if not apikey:
+            self.raise_empty_file(filename)
 
+        self.api_key = apikey
 
-def read_key_from_environment_variable():
-    apikey = os.environ.get(NASDAQ_DATA_LINK_API_KEY)
-    if not apikey:
-        raise_empty_environment_variable()
+    def api_key_environment_variable_exists(self):
+        return NASDAQ_DATA_LINK_API_KEY in os.environ
 
-    ApiConfig.api_key = apikey
+    def read_key_from_environment_variable(self):
+        apikey = os.environ.get(NASDAQ_DATA_LINK_API_KEY)
+        if not apikey:
+            self.raise_empty_environment_variable()
 
+        self.api_key = apikey
 
-def read_key(filename=None):
-    if api_key_environment_variable_exists():
-        read_key_from_environment_variable()
-    elif config_file_exists(filename):
-        read_key_from_file(filename)
+    def read_key(self, filename=None):
+        if self.api_key_environment_variable_exists():
+            self.read_key_from_environment_variable()
+        elif self.config_file_exists(filename):
+            self.read_key_from_file(filename)
 
 
 def get_config_from_kwargs(kwargs):
-    result = ApiConfig
     if isinstance(kwargs, dict):
         params = kwargs.get('params')
+
         if isinstance(params, dict):
             result = params.get('api_config')
+
             if not isinstance(result, ApiConfig):
-                result = ApiConfig
-    return result
+                return DEFAULT_API_CONFIG
+            return result
+
+    return DEFAULT_API_CONFIG
+
+
+DEFAULT_API_CONFIG = ApiConfig()

nasdaqdatalink/model/authorized_session.py

@@ -1,12 +1,12 @@
 import nasdaqdatalink
-from nasdaqdatalink.api_config import ApiConfig
+from nasdaqdatalink.api_config import ApiConfig, DEFAULT_API_CONFIG
 from urllib3.util.retry import Retry
 from requests.adapters import HTTPAdapter
 import requests
 import urllib
 
 
-def get_retries(api_config=nasdaqdatalink.ApiConfig):
+def get_retries(api_config=DEFAULT_API_CONFIG):
     retries = None
     if not api_config.use_retries:
         return Retry(total=0)
@@ -22,10 +22,10 @@ def get_retries(api_config=nasdaqdatalink.ApiConfig):
 
 
 class AuthorizedSession:
-    def __init__(self, api_config=ApiConfig) -> None:
+    def __init__(self, api_config=DEFAULT_API_CONFIG) -> None:
         super(AuthorizedSession, self).__init__()
         if not isinstance(api_config, ApiConfig):
-            api_config = ApiConfig
+            api_config = DEFAULT_API_CONFIG
         self._api_config = api_config
         self._auth_session = requests.Session()
         retries = get_retries(self._api_config)

test/test_api_config.py

@@ -43,70 +43,75 @@ def tearDown(self):
 
     def test_read_key_when_environment_variable_set(self):
         os.environ['NASDAQ_DATA_LINK_API_KEY'] = 'setinenv'
-        ApiConfig.api_key = None
-        read_key()
-        self.assertEqual(ApiConfig.api_key, "setinenv")
+        api_config = ApiConfig()
+        self.assertEqual(api_config.api_key, "setinenv")
 
 
     def test_read_key_environment_variable_takes_precedence(self):
         os.environ['NASDAQ_DATA_LINK_API_KEY'] = 'setinenvprecedence'
-        save_key("keyforfilenot", TEST_KEY_FILE)
-        ApiConfig.api_key = None
-        read_key()
-        self.assertEqual(ApiConfig.api_key, "setinenvprecedence")
+        api_config = ApiConfig()
+        api_config.save_key("keyforfilenot", TEST_KEY_FILE)
+        api_config.api_key = None
+        api_config.read_key()
+        self.assertEqual(api_config.api_key, "setinenvprecedence")
 
 
     def test_read_key_when_environment_variable_not_set(self):
-        save_key("keyforfile", TEST_KEY_FILE)
-        ApiConfig.api_key = None # Set None, we are not testing save_key
-        read_key(TEST_KEY_FILE)
-        self.assertEqual(ApiConfig.api_key, 'keyforfile')
+        api_config = ApiConfig()
+        api_config.save_key("keyforfile", TEST_KEY_FILE)
+        api_config.api_key = None # Set None, we are not testing save_key
+        api_config.read_key(TEST_KEY_FILE)
+        self.assertEqual(api_config.api_key, 'keyforfile')
 
 
     def test_read_key_empty_file(self):
-        with mock.patch("nasdaqdatalink.api_config.default_config_filename") as mock_default_config_filename:
+        with mock.patch("nasdaqdatalink.api_config.ApiConfig.default_config_filename") as mock_default_config_filename:
             mock_default_config_filename.return_value = TEST_DEFAULT_FILE
-            save_key("")
+            api_config = ApiConfig()
+            api_config.save_key("")
             with self.assertRaises(ValueError):
-                read_key()
+                api_config.read_key()
 
 
     def test_read_key_when_env_key_empty(self):
         os.environ['NASDAQ_DATA_LINK_API_KEY'] = ''
         with self.assertRaises(ValueError):
-            read_key()
+            ApiConfig().read_key()
 
 
     def test_read_key_when_files_not_set(self):
-        ApiConfig.api_key = None
-        with mock.patch("nasdaqdatalink.api_config.default_config_filename") as mock_default_config_filename:
+        api_config = ApiConfig()
+        api_config.api_key = None
+        with mock.patch("nasdaqdatalink.api_config.ApiConfig.default_config_filename") as mock_default_config_filename:
             mock_default_config_filename.return_value = TEST_DEFAULT_FILE
-            read_key()
+            api_config.read_key()
 
         mock_default_config_filename.assert_called_once
-        self.assertEqual(ApiConfig.api_key, None)
+        self.assertEqual(api_config.api_key, None)
 
 
     def test_read_key_when_default_file_set(self):
-        save_key("keyfordefaultfile", TEST_DEFAULT_FILE)
-        ApiConfig.api_key = None # Set None, we are not testing save_key
+        api_config = ApiConfig()
+        api_config.save_key("keyfordefaultfile", TEST_DEFAULT_FILE)
+        api_config.api_key = None # Set None, we are not testing save_key
 
-        with mock.patch("nasdaqdatalink.api_config.default_config_filename") as mock_default_config_filename:
+        with mock.patch("nasdaqdatalink.api_config.ApiConfig.default_config_filename") as mock_default_config_filename:
             mock_default_config_filename.return_value = TEST_DEFAULT_FILE
-            read_key()
+            api_config.read_key()
 
-        self.assertEqual(ApiConfig.api_key, 'keyfordefaultfile')
+        self.assertEqual(api_config.api_key, 'keyfordefaultfile')
 
 
     def _read_key_from_file_helper(self, given, expected):
-        save_key(given, TEST_DEFAULT_FILE)
-        ApiConfig.api_key = None # Set None, we are not testing save_key
+        api_config = ApiConfig()
+        api_config.save_key(given, TEST_DEFAULT_FILE)
+        api_config.api_key = None # Set None, we are not testing save_key
 
-        with mock.patch("nasdaqdatalink.api_config.default_config_filename") as mock_default_config_filename:
+        with mock.patch("nasdaqdatalink.api_config.ApiConfig.default_config_filename") as mock_default_config_filename:
             mock_default_config_filename.return_value = TEST_DEFAULT_FILE
-            read_key()
+            api_config.read_key()
 
-        self.assertEqual(ApiConfig.api_key, expected)
+        self.assertEqual(api_config.api_key, expected)
 
 
     def test_read_key_from_file_with_newline(self):
@@ -136,12 +141,12 @@ def test_read_key_from_file_with_multi_newline(self):
     def test_default_instance_will_have_share_values_with_singleton(self):
         os.environ['NASDAQ_DATA_LINK_API_KEY'] = 'setinenv'
         ApiConfig.api_key = None
-        read_key()
         api_config = ApiConfig()
+        api_config.read_key()
         self.assertEqual(api_config.api_key, "setinenv")
         # make sure change in instance will not affect the singleton
-        api_config.api_key = None
-        self.assertEqual(ApiConfig.api_key, "setinenv")
+        api_config.api_key = 'foo'
+        self.assertIsNone(ApiConfig.api_key)
 
     def test_get_config_from_kwargs_return_api_config_if_present(self):
         api_config = get_config_from_kwargs({
@@ -164,26 +169,17 @@ def test_get_config_from_kwargs_return_singleton_if_not_present_or_wrong_type(se
         self.assertTrue(issubclass(api_config, ApiConfig))
         self.assertFalse(isinstance(api_config, ApiConfig))
 
-    def test_instance_read_key_should_raise_error(self):
-        api_config = ApiConfig()
-        with self.assertRaises(TypeError):
-            api_config.read_key(None)
-        with self.assertRaises(ValueError):
-            api_config.read_key('')
-
     def test_instance_read_key_should_raise_error_when_empty(self):
-        save_key("", TEST_KEY_FILE)
         api_config = ApiConfig()
+        api_config.save_key("", TEST_KEY_FILE)
         with self.assertRaises(ValueError):
             # read empty file
             api_config.read_key(TEST_KEY_FILE)
 
     def test_instance_read_the_right_key(self):
         expected_key = 'ilovepython'
-        save_key(expected_key, TEST_KEY_FILE)
         api_config = ApiConfig()
+        api_config.save_key(expected_key, TEST_KEY_FILE)
         api_config.api_key = ''
         api_config.read_key(TEST_KEY_FILE)
-        self.assertEqual(ApiConfig.api_key, expected_key)
-        
-        
+        self.assertEqual(api_config.api_key, expected_key)