-
Notifications
You must be signed in to change notification settings - Fork 38
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
Tdl 14803 check api access in discovery mode #74
base: master
Are you sure you want to change the base?
Changes from 48 commits
21edee7
bc279a5
7663264
475719c
7a5fd9b
f8cf58b
1b9d70a
88a3760
080b2ad
3b71bea
125a051
7bfdd1e
e409fae
46855f1
95e4dee
f10650e
15d5c64
961fcb1
75363da
c47824f
ee826c5
3313e58
543b960
db3c697
3af90b6
bd4771e
5eaf72e
5770b07
17757a0
b376cde
0f7968c
c3fb796
c379ad2
081060e
929f5d7
b168865
89be5f4
b509cf6
269ca0c
d08b811
bd1fd48
57daa7b
0c4b706
d87d99e
8cd80ff
ef85e74
ef19a71
33ac8d7
d630fca
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 |
---|---|---|
@@ -1,7 +1,11 @@ | ||
import os | ||
import json | ||
import singer | ||
import zenpy | ||
from tap_zendesk.streams import STREAMS | ||
from tap_zendesk.http import ZendeskForbiddenError | ||
|
||
LOGGER = singer.get_logger() | ||
|
||
def get_abs_path(path): | ||
return os.path.join(os.path.dirname(os.path.realpath(__file__)), path) | ||
|
@@ -20,12 +24,43 @@ def load_shared_schema_refs(): | |
|
||
return shared_schema_refs | ||
|
||
def discover_streams(client): | ||
def discover_streams(client, config): | ||
streams = [] | ||
error_list = [] | ||
refs = load_shared_schema_refs() | ||
|
||
|
||
for s in STREAMS.values(): | ||
s = s(client) | ||
s = s(client, config) | ||
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. @prijendev Can you please give understandable variable name instead of 's' 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. In existing code they have used 's'. Change in name will reflect lot off changes in code as it is used in many place. 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. @prijendev Please do the variable name changes. If it means change will reflect lot off changes that's fine 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. @prijendev Add Comments to the code 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.
Added comments to the code. And you can find detailed comments in the |
||
schema = singer.resolve_schema_references(s.load_schema(), refs) | ||
try: | ||
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. Add comments here to explain the code. 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. Added |
||
# Here it call the check_access method to check whether stream have read permission or not. | ||
# If stream does not have read permission then append that stream name to list and at the end of all streams | ||
# raise forbidden error with proper message containing stream names. | ||
s.check_access() | ||
except ZendeskForbiddenError as e: | ||
error_list.append(s.name) # Append stream name to the error_list | ||
except zenpy.lib.exception.APIException as e: | ||
args0 = json.loads(e.args[0]) | ||
err = args0.get('error') | ||
|
||
# check if the error is of type dictionary and the message retrieved from the dictionary | ||
# is the expected message. If so, only then print the logger message and return the schema | ||
if isinstance(err, dict): | ||
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. @prijendev Can you add same comment here as it is added in this PR - https://github.com/singer-io/tap-zendesk/pull/69/files#diff-89887ca52395ce152e9723c5595655edcd41793204060336e88ea9805c274433R142 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. Updated |
||
if err.get('message', None) == "You do not have access to this page. Please contact the account owner of this help desk for further help.": | ||
error_list.append(s.name) | ||
elif args0.get('description') == "You are missing the following required scopes: read": | ||
error_list.append(s.name) | ||
else: | ||
raise e from None # raise error if it is other than 403 forbidden error | ||
|
||
streams.append({'stream': s.name, 'tap_stream_id': s.name, 'schema': schema, 'metadata': s.load_metadata()}) | ||
|
||
if error_list: | ||
streams_name = ", ".join(error_list) | ||
message = "HTTP-error-code: 403, Error: You are missing the following required scopes: read. "\ | ||
"The account credentials supplied do not have read access for the following stream(s): {}".format(streams_name) | ||
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. @prijendev As per the best practices, we should not use 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. They don't updated python to the latest version. That's why |
||
raise ZendeskForbiddenError(message) | ||
|
||
|
||
return streams |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,32 +2,154 @@ | |
import backoff | ||
import requests | ||
import singer | ||
from requests.exceptions import Timeout, HTTPError | ||
|
||
|
||
|
||
LOGGER = singer.get_logger() | ||
|
||
|
||
class ZendeskError(Exception): | ||
def __init__(self, message=None, response=None): | ||
super().__init__(message) | ||
self.message = message | ||
self.response = response | ||
|
||
class ZendeskBackoffError(ZendeskError): | ||
pass | ||
|
||
class ZendeskBadRequestError(ZendeskError): | ||
pass | ||
|
||
class ZendeskUnauthorizedError(ZendeskError): | ||
pass | ||
|
||
class ZendeskForbiddenError(ZendeskError): | ||
pass | ||
|
||
class ZendeskNotFoundError(ZendeskError): | ||
pass | ||
|
||
class ZendeskConflictError(ZendeskError): | ||
pass | ||
|
||
class ZendeskUnprocessableEntityError(ZendeskError): | ||
pass | ||
|
||
class ZendeskRateLimitError(ZendeskBackoffError): | ||
pass | ||
|
||
class ZendeskInternalServerError(ZendeskBackoffError): | ||
pass | ||
|
||
class ZendeskNotImplementedError(ZendeskBackoffError): | ||
pass | ||
|
||
class ZendeskBadGatewayError(ZendeskBackoffError): | ||
pass | ||
|
||
class ZendeskServiceUnavailableError(ZendeskBackoffError): | ||
pass | ||
|
||
ERROR_CODE_EXCEPTION_MAPPING = { | ||
400: { | ||
"raise_exception": ZendeskBadRequestError, | ||
"message": "A validation exception has occurred." | ||
}, | ||
401: { | ||
"raise_exception": ZendeskUnauthorizedError, | ||
"message": "The access token provided is expired, revoked, malformed or invalid for other reasons." | ||
}, | ||
403: { | ||
"raise_exception": ZendeskForbiddenError, | ||
"message": "You are missing the following required scopes: read" | ||
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. Will it be each time 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. Yes. Because we are calling just GET API for each stream. So, it will return the same message of read scopes. |
||
}, | ||
404: { | ||
"raise_exception": ZendeskNotFoundError, | ||
"message": "The resource you have specified cannot be found." | ||
}, | ||
409: { | ||
"raise_exception": ZendeskConflictError, | ||
"message": "The API request cannot be completed because the requested operation would conflict with an existing item." | ||
}, | ||
422: { | ||
"raise_exception": ZendeskUnprocessableEntityError, | ||
"message": "The request content itself is not processable by the server." | ||
}, | ||
429: { | ||
"raise_exception": ZendeskRateLimitError, | ||
"message": "The API rate limit for your organisation/application pairing has been exceeded." | ||
}, | ||
500: { | ||
"raise_exception": ZendeskInternalServerError, | ||
"message": "The server encountered an unexpected condition which prevented" \ | ||
" it from fulfilling the request." | ||
}, | ||
501: { | ||
"raise_exception": ZendeskNotImplementedError, | ||
"message": "The server does not support the functionality required to fulfill the request." | ||
}, | ||
502: { | ||
"raise_exception": ZendeskBadGatewayError, | ||
"message": "Server received an invalid response." | ||
}, | ||
503: { | ||
"raise_exception": ZendeskServiceUnavailableError, | ||
"message": "API service is currently unavailable." | ||
} | ||
} | ||
def is_fatal(exception): | ||
status_code = exception.response.status_code | ||
|
||
if status_code == 429: | ||
sleep_time = int(exception.response.headers['Retry-After']) | ||
LOGGER.info("Caught HTTP 429, retrying request in %s seconds", sleep_time) | ||
sleep(sleep_time) | ||
return False | ||
|
||
return 400 <= status_code < 500 | ||
if status_code in [429, 503]: | ||
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. Please add comments here!! 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. Added. |
||
# If status_code is 429 or 503 then checking whether response header has 'Retry-After' attribute or not. | ||
# If response header has 'Retry-After' attribute then retry the error otherwise raise the error directly. | ||
retry_after = exception.response.headers.get('Retry-After') | ||
if retry_after: | ||
sleep_time = int(retry_after) | ||
LOGGER.info("Caught HTTP %s, retrying request in %s seconds", status_code, sleep_time) | ||
sleep(sleep_time) | ||
return False | ||
else: | ||
return True | ||
|
||
return 400 <=status_code < 500 | ||
|
||
def raise_for_error(response): | ||
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. Please add comments for this function. 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. added comments. |
||
""" Error handling method which throws custom error. Class for each error defined above which extends `ZendeskError`. | ||
This method map the status code with `ERROR_CODE_EXCEPTION_MAPPING` dictionary and accordingly raise the error. | ||
If status_code is 200 then simply return json response. | ||
""" | ||
try: | ||
response_json = response.json() | ||
except Exception: # pylint: disable=broad-except | ||
response_json = {} | ||
if response.status_code != 200: | ||
if response_json.get('error'): | ||
message = "HTTP-error-code: {}, Error: {}".format(response.status_code, response_json.get('error')) | ||
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. @prijendev Don't use 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. They don't updated python to the latest version. That's why |
||
else: | ||
message = "HTTP-error-code: {}, Error: {}".format( | ||
response.status_code, | ||
response_json.get("message", ERROR_CODE_EXCEPTION_MAPPING.get( | ||
response.status_code, {}).get("message", "Unknown Error"))) | ||
exc = ERROR_CODE_EXCEPTION_MAPPING.get( | ||
response.status_code, {}).get("raise_exception", ZendeskError) | ||
raise exc(message, response) from None | ||
|
||
@backoff.on_exception(backoff.expo, | ||
requests.exceptions.HTTPError, | ||
(HTTPError, ZendeskBackoffError), | ||
max_tries=10, | ||
giveup=is_fatal) | ||
def call_api(url, params, headers): | ||
response = requests.get(url, params=params, headers=headers) | ||
response.raise_for_status() | ||
@backoff.on_exception(backoff.expo, | ||
(ConnectionError, Timeout),#As ConnectionError error and timeout error does not have attribute status_code, | ||
max_tries=10, # here we added another backoff expression. | ||
factor=2) | ||
def call_api(url, request_timeout, params, headers): | ||
response = requests.get(url, params=params, headers=headers, timeout=request_timeout) # Pass request timeout | ||
raise_for_error(response) | ||
return response | ||
|
||
def get_cursor_based(url, access_token, cursor=None, **kwargs): | ||
def get_cursor_based(url, access_token, request_timeout, cursor=None, **kwargs): | ||
headers = { | ||
'Content-Type': 'application/json', | ||
'Accept': 'application/json', | ||
|
@@ -43,7 +165,7 @@ def get_cursor_based(url, access_token, cursor=None, **kwargs): | |
if cursor: | ||
params['page[after]'] = cursor | ||
|
||
response = call_api(url, params=params, headers=headers) | ||
response = call_api(url, request_timeout, params=params, headers=headers) | ||
response_json = response.json() | ||
|
||
yield response_json | ||
|
@@ -54,13 +176,13 @@ def get_cursor_based(url, access_token, cursor=None, **kwargs): | |
cursor = response_json['meta']['after_cursor'] | ||
params['page[after]'] = cursor | ||
|
||
response = call_api(url, params=params, headers=headers) | ||
response = call_api(url, request_timeout, params=params, headers=headers) | ||
response_json = response.json() | ||
|
||
yield response_json | ||
has_more = response_json['meta']['has_more'] | ||
|
||
def get_offset_based(url, access_token, **kwargs): | ||
def get_offset_based(url, access_token, request_timeout, **kwargs): | ||
headers = { | ||
'Content-Type': 'application/json', | ||
'Accept': 'application/json', | ||
|
@@ -73,21 +195,21 @@ def get_offset_based(url, access_token, **kwargs): | |
**kwargs.get('params', {}) | ||
} | ||
|
||
response = call_api(url, params=params, headers=headers) | ||
response = call_api(url, request_timeout, params=params, headers=headers) | ||
response_json = response.json() | ||
|
||
yield response_json | ||
|
||
next_url = response_json.get('next_page') | ||
|
||
while next_url: | ||
response = call_api(next_url, params=None, headers=headers) | ||
response = call_api(next_url, request_timeout, params=None, headers=headers) | ||
response_json = response.json() | ||
|
||
yield response_json | ||
next_url = response_json.get('next_page') | ||
|
||
def get_incremental_export(url, access_token, start_time): | ||
def get_incremental_export(url, access_token, request_timeout, start_time): | ||
headers = { | ||
'Content-Type': 'application/json', | ||
'Accept': 'application/json', | ||
|
@@ -96,7 +218,7 @@ def get_incremental_export(url, access_token, start_time): | |
|
||
params = {'start_time': start_time.timestamp()} | ||
|
||
response = call_api(url, params=params, headers=headers) | ||
response = call_api(url, request_timeout, params=params, headers=headers) | ||
response_json = response.json() | ||
|
||
yield response_json | ||
|
@@ -107,8 +229,13 @@ def get_incremental_export(url, access_token, start_time): | |
cursor = response_json['after_cursor'] | ||
|
||
params = {'cursor': cursor} | ||
response = requests.get(url, params=params, headers=headers) | ||
response.raise_for_status() | ||
# Replaced below line of code with call_api method | ||
# response = requests.get(url, params=params, headers=headers) | ||
# response.raise_for_status() | ||
# Because it doing the same as call_api. So, now error handling will work properly with backoff | ||
# as earlier backoff was not possible | ||
response = call_api(url, request_timeout, params=params, headers=headers) | ||
|
||
response_json = response.json() | ||
|
||
yield response_json | ||
|
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.
Please add comments to the code changes
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.
Added comments