Skip to content
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

Open
wants to merge 49 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 48 commits
Commits
Show all changes
49 commits
Select commit Hold shift + click to select a range
21edee7
API call to the each stream in discovery mode done
prijendev Sep 24, 2021
bc279a5
removed generated catalog file
prijendev Sep 24, 2021
7663264
resolved pylint errors
prijendev Sep 24, 2021
475719c
Resolved cyclic import pylint error
prijendev Sep 27, 2021
7a5fd9b
Improved unittest case civerage
prijendev Sep 27, 2021
f8cf58b
Updated error message for 403 forbidden error
prijendev Sep 28, 2021
1b9d70a
Updated error handling
prijendev Sep 30, 2021
88a3760
resolved pylint error
prijendev Sep 30, 2021
080b2ad
Removed empty catalog
prijendev Sep 30, 2021
3b71bea
Removed unused catalog file.
prijendev Sep 30, 2021
125a051
Removed unused state file
prijendev Sep 30, 2021
7bfdd1e
Removed unused state file
prijendev Sep 30, 2021
e409fae
Removed unused file
prijendev Sep 30, 2021
46855f1
Updated error message and unittest case for 404 error
prijendev Oct 4, 2021
95e4dee
Merge branch 'TDL-14803-check-api-access-in-discovery-mode' of https:…
prijendev Oct 4, 2021
f10650e
Resolved merge conflict
prijendev Oct 11, 2021
15d5c64
Updated check access method
prijendev Oct 11, 2021
961fcb1
Resolved pylint error
prijendev Oct 12, 2021
75363da
recolved unused argument error
prijendev Oct 12, 2021
c47824f
resolved kwargs error
prijendev Oct 12, 2021
ee826c5
Updated unittest cases
prijendev Oct 12, 2021
3313e58
Updated unittest cases
prijendev Oct 13, 2021
543b960
Removed global variable
prijendev Oct 13, 2021
db3c697
Improved unittest case coverage
prijendev Oct 13, 2021
3af90b6
updated 404 error
prijendev Oct 13, 2021
bd4771e
resolved pylint error
prijendev Oct 13, 2021
5eaf72e
Updated typo error.
prijendev Oct 14, 2021
5770b07
Removed f strings
prijendev Oct 18, 2021
17757a0
Merge branch 'TDL-14803-check-api-access-in-discovery-mode' of https:…
prijendev Oct 18, 2021
b376cde
Updated error handling
prijendev Oct 18, 2021
0f7968c
resloved pylint error
prijendev Oct 18, 2021
c3fb796
resolved unittest case error
prijendev Oct 18, 2021
c379ad2
Added more comments and updated code
prijendev Oct 19, 2021
081060e
resolved pylint error
prijendev Oct 19, 2021
929f5d7
updated method name
prijendev Oct 19, 2021
b168865
Added timeout error code
prijendev Oct 20, 2021
89be5f4
Resolved pylint error
prijendev Oct 20, 2021
b509cf6
added coverage report to artifact
prijendev Oct 21, 2021
269ca0c
added pylint back
prijendev Oct 21, 2021
d08b811
Added comment
prijendev Oct 28, 2021
bd1fd48
resolved pylint errors
prijendev Oct 28, 2021
57daa7b
Enhanced the code
prijendev Nov 1, 2021
0c4b706
Reutilized args0
prijendev Nov 1, 2021
d87d99e
Moved request_timeout parameter to common class
prijendev Nov 2, 2021
8cd80ff
Added comment
prijendev Nov 2, 2021
ef85e74
Removed static time
prijendev Nov 2, 2021
ef19a71
removed warning message
prijendev Nov 2, 2021
33ac8d7
resolved pylint error
prijendev Nov 2, 2021
d630fca
resolved the comments
namrata270998 Nov 3, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion .circleci/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,19 @@ jobs:
virtualenv -p python3 /usr/local/share/virtualenvs/tap-zendesk
source /usr/local/share/virtualenvs/tap-zendesk/bin/activate
pip install .[test]
pip install coverage
- run:
name: 'pylint'
command: |
source /usr/local/share/virtualenvs/tap-zendesk/bin/activate
make test
pylint tap_zendesk -d missing-docstring,invalid-name,line-too-long,too-many-locals,too-few-public-methods,fixme,stop-iteration-return,too-many-branches,useless-import-alias,no-else-return,logging-not-lazy
nosetests --with-coverage --cover-erase --cover-package=tap_zendesk --cover-html-dir=htmlcov test/unittests
coverage html
- add_ssh_keys
- store_test_results:
path: test_output/report.xml
- store_artifacts:
path: htmlcov
- run:
name: 'Integration Tests'
command: |
Expand Down
7 changes: 4 additions & 3 deletions tap_zendesk/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@

LOGGER = singer.get_logger()


REQUIRED_CONFIG_KEYS = [
"start_date",
"subdomain",
Expand Down Expand Up @@ -46,9 +47,9 @@ def request_metrics_patch(self, method, url, **kwargs):
Session.request = request_metrics_patch
# end patch

def do_discover(client):
def do_discover(client, config):
LOGGER.info("Starting discover")
catalog = {"streams": discover_streams(client)}
catalog = {"streams": discover_streams(client, config)}
json.dump(catalog, sys.stdout, indent=2)
LOGGER.info("Finished discover")

Expand Down Expand Up @@ -199,7 +200,7 @@ def main():
LOGGER.error("""No suitable authentication keys provided.""")

if parsed_args.discover:
do_discover(client)
do_discover(client, parsed_args.config)
Copy link
Contributor

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

Copy link
Contributor

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

Added comments

elif parsed_args.catalog:
state = parsed_args.state
do_sync(client, parsed_args.catalog, state, parsed_args.config)
39 changes: 37 additions & 2 deletions tap_zendesk/discover.py
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)
Expand All @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@prijendev Can you please give understandable variable name instead of 's'

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@prijendev Add Comments to the code

Copy link
Contributor

@namrata270998 namrata270998 Nov 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@prijendev Add Comments to the code

Added comments to the code. And you can find detailed comments in the try block as well. Also updated the variable name s to stream

schema = singer.resolve_schema_references(s.load_schema(), refs)
try:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add comments here to explain the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@prijendev As per the best practices, we should not use format(). Please update this code

Copy link
Contributor Author

@prijendev prijendev Nov 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They don't updated python to the latest version. That's why format() has been kept.

raise ZendeskForbiddenError(message)


return streams
169 changes: 148 additions & 21 deletions tap_zendesk/http.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will it be each time read only? If No then please put a generalized message.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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]:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add comments here!!

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add comments for this function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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'))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@prijendev Don't use format() in the code

Copy link
Contributor Author

@prijendev prijendev Nov 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They don't updated python to the latest version. That's why format() has been kept.

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',
Expand All @@ -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
Expand All @@ -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',
Expand All @@ -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',
Expand All @@ -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
Expand All @@ -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
Expand Down
Loading