-
Notifications
You must be signed in to change notification settings - Fork 78
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add AuthorizedSession #22
base: main
Are you sure you want to change the base?
Changes from all commits
7fb9f10
784a11f
bfaa3b0
b7dca2b
01e4a11
1a990da
78cacae
e87eee2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,6 +17,18 @@ class ApiConfig: | |
retry_status_codes = [429] + list(range(500, 512)) | ||
verify_ssl = True | ||
|
||
def read_key(self, filename=None): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this only invoked in the tests? |
||
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) | ||
|
||
self.api_key = apikey | ||
|
||
|
||
def create_file(config_filename): | ||
# Create the file as well as the parent dir if needed. | ||
|
@@ -102,3 +114,14 @@ def read_key(filename=None): | |
read_key_from_environment_variable() | ||
elif config_file_exists(filename): | ||
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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,57 @@ | ||
import nasdaqdatalink | ||
from nasdaqdatalink.api_config import ApiConfig | ||
from urllib3.util.retry import Retry | ||
from requests.adapters import HTTPAdapter | ||
import requests | ||
import urllib | ||
|
||
|
||
def get_retries(api_config=nasdaqdatalink.ApiConfig): | ||
retries = None | ||
if not api_config.use_retries: | ||
return Retry(total=0) | ||
|
||
Retry.BACKOFF_MAX = api_config.max_wait_between_retries | ||
retries = Retry(total=api_config.number_of_retries, | ||
connect=api_config.number_of_retries, | ||
read=api_config.number_of_retries, | ||
status_forcelist=api_config.retry_status_codes, | ||
backoff_factor=api_config.retry_backoff_factor, | ||
raise_on_status=False) | ||
return retries | ||
|
||
|
||
class AuthorizedSession: | ||
def __init__(self, api_config=ApiConfig) -> None: | ||
super(AuthorizedSession, self).__init__() | ||
if not isinstance(api_config, ApiConfig): | ||
api_config = ApiConfig | ||
self._api_config = api_config | ||
self._auth_session = requests.Session() | ||
retries = get_retries(self._api_config) | ||
adapter = HTTPAdapter(max_retries=retries) | ||
self._auth_session.mount(api_config.api_protocol, adapter) | ||
|
||
proxies = urllib.request.getproxies() | ||
if proxies is not None: | ||
self._auth_session.proxies.update(proxies) | ||
|
||
def get(self, dataset, **kwargs): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Outside of this tickets scope, but I would like to see us passing connection details (session, api_config) as explicit arguments, and leave kwargs to params we should send to the API call. This would allow us to avoid all the kwarg popping down the line |
||
nasdaqdatalink.get(dataset, session=self._auth_session, | ||
api_config=self._api_config, **kwargs) | ||
|
||
def bulkdownload(self, database, **kwargs): | ||
nasdaqdatalink.bulkdownload(database, session=self._auth_session, | ||
api_config=self._api_config, **kwargs) | ||
|
||
def export_table(self, datatable_code, **kwargs): | ||
nasdaqdatalink.export_table(datatable_code, session=self._auth_session, | ||
api_config=self._api_config, **kwargs) | ||
|
||
def get_table(self, datatable_code, **options): | ||
nasdaqdatalink.get_table(datatable_code, session=self._auth_session, | ||
api_config=self._api_config, **options) | ||
|
||
def get_point_in_time(self, datatable_code, **options): | ||
nasdaqdatalink.get_point_in_time(datatable_code, session=self._auth_session, | ||
api_config=self._api_config, **options) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is adding read_key as a method to ApiConfig necessary? I'm not sure it's ever used. If users define their own session and pass in ApiConfig object, how should the api key poking be reconciled? It appears this ApiConfig.read_key() only considers file-based API Keys, and is never called anywhere -- am I missing something?
__init__
will call read_key() and kick off how to read the user's key from Env or a file, preferring ENV vars first.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function should be called
read_key_from_file
. I was thinking it would be nice to allow each ApiConfig instances to read keys from different files.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, but even so it still duplicates the namespace method, which I'd like to avoid having multiple methods that do the same thing. Ideally the behaviour for finding a key is to:
I didn't catch any logic that attempted to call ApiConfig.read_key() or what you propose should be ApiConfig.read_key_from_file() -- so I'm curious whether that's the correct level to provide those helper methods.
ApiConfig is just being used as a dataclass anyway, and at some point in the future we should convert it to one. We can have helper methods in it, for sure, but we need to make sure that it should be responsible for doing those tasks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i was think something like this:
Basically, i was trying to avoid using ApiConfig as a mutable storage of global state, but I understand it does introduce additional complexity to the users by enforcing a context creation beforehand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, we'd need to go through some trouble to make ApiConfig immutable. I'm okay with the mutability of ApiConfig, since we should provide a consistent means of allowing users to set their api key if they need more configuration options.
ApiConfig() on create should do heavy lifting of finding API Key in env var or in a file, using defaults.
We should provide convenience methods to allow callers to if they wish to override defaults. We should encourage people to simply use the defaults.
We can modify
__init__
to no longer call read_key() as this must being taken care of for callers: if one does not define api_config kw-arg then use our default object state.We could move the helper methods into ApiConfig instead and avoid duplicating definitions. I would prefer we have consistent logic in one place, rather than maintain multiple definitions of the same logic.