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 6 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
6 changes: 3 additions & 3 deletions tap_zendesk/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -46,9 +46,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 +199,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)
33 changes: 31 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,37 @@ 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

s.check_access()
except ZendeskForbiddenError as e:
error_list.append(s.name)
except zenpy.lib.exception.APIException as e:
err = json.loads(e.args[0]).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 Since e.args[0] is being used multiple times. Please make it a variable and get it converted in the json.loads. and re-use it further everywhere

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 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 json.loads(e.args[0]).get('description') == "You are missing the following required scopes: read":
error_list.append(s.name)
else:
raise e
Copy link
Contributor

Choose a reason for hiding this comment

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

@prijendev Raise an exception from None.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Raised an exception from None.


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. "\
f"The account credentials supplied do not have read access for the following stream(s): {streams_name}"
raise ZendeskForbiddenError(message)


return streams
115 changes: 112 additions & 3 deletions tap_zendesk/http.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,95 @@
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(ZendeskError):
pass

class ZendeskNotImplementedError(ZendeskError):
pass

class ZendeskBadGatewayError(ZendeskError):
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": "There is no help desk configured at this address. This means that the address is available and that you can claim it at http://www.zendesk.com/signup"
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above if it is not the fixed message for 404 each time 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.

Updated to generalized message.

},
409: {
"raise_exception": ZendeskConflictError,
"message": "The request does not match our state in some way."
},
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

Expand All @@ -15,16 +104,36 @@ def is_fatal(exception):
LOGGER.info("Caught HTTP 429, retrying request in %s seconds", sleep_time)
sleep(sleep_time)
return False

elif status_code == 503:
sleep_time = int(exception.response.headers['Retry-After'])
LOGGER.info("Caught HTTP 503, retrying request in %s seconds", sleep_time)
sleep(sleep_time)
return False
return 400 <=status_code < 500

def check_status(response):
# Forming a response message for raising custom exception
try:
response_json = response.json()
except Exception: # pylint: disable=broad-except
response_json = {}
if response.status_code != 200:
message = "HTTP-error-code: {}, Error: {}".format(
Copy link
Contributor

Choose a reason for hiding this comment

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

Use f-strings instead of format.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replaced format with f-string.

response.status_code,
response_json.get("errorMessages", [ERROR_CODE_EXCEPTION_MAPPING.get(
response.status_code, {}).get("message", "Unknown Error")])[0]
)
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,
ZendeskBackoffError,
max_tries=10,
giveup=is_fatal)
def call_api(url, params, headers):
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.

response = requests.get(url, params=params, headers=headers)
response.raise_for_status()
check_status(response)
return response


Expand Down
43 changes: 43 additions & 0 deletions tap_zendesk/streams.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
LOGGER = singer.get_logger()
KEY_PROPERTIES = ['id']

START_DATE = datetime.datetime.strftime(datetime.datetime.utcnow() - datetime.timedelta(days=1), "%Y-%m-%dT00:00:00Z")
CUSTOM_TYPES = {
'text': 'string',
'textarea': 'string',
Expand Down Expand Up @@ -114,13 +115,33 @@ def load_metadata(self):
def is_selected(self):
return self.stream is not None

def check_access(self):
'''
Check whether permission given to access stream resource or not.
Copy link
Contributor

@dbshah1212 dbshah1212 Sep 30, 2021

Choose a reason for hiding this comment

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

Suggested change
Check whether permission given to access stream resource or not.
Check whether the permission was given to access stream resources or not.

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.

'''
url = self.endpoint.format(self.config['subdomain'])

headers = {
'Content-Type': 'application/json',
'Accept': 'application/json',
'Authorization': 'Bearer {}'.format(self.config['access_token'])
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
headers = {
'Content-Type': 'application/json',
'Accept': 'application/json',
'Authorization': 'Bearer {}'.format(self.config['access_token'])
}
headers = {
'Content-Type': 'application/json',
'Accept': 'application/json',
'Authorization': f'Bearer {self.config['access_token']}'
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replaced format with f-string.


http.call_api(url, params={'page[size]': 1}, headers=headers)


def raise_or_log_zenpy_apiexception(schema, stream, e):
# There are multiple tiers of Zendesk accounts. Some of them have
# access to `custom_fields` and some do not. This is the specific
# error that appears to be return from the API call in the event that
# it doesn't have access.
if not isinstance(e, zenpy.lib.exception.APIException):
raise ValueError("Called with a bad exception type") from e
#If read permission not available in oauth access_token, then it returns below error.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#If read permission not available in oauth access_token, then it returns below error.
#If read permission is not available in OAuth access_token, then it returns the below error.

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 json.loads(e.args[0]).get('description') == "You are missing the following required scopes: read":
LOGGER.warning("The account credentials supplied do not have access to `%s` custom fields.",
stream)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
LOGGER.warning("The account credentials supplied do not have access to `%s` custom fields.",
stream)
LOGGER.warning("The account credentials supplied do not have access to `%s` custom fields.",
stream)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved

return schema
if json.loads(e.args[0])['error']['message'] == "You do not have access to this page. Please contact the account owner of this help desk for further help.":
LOGGER.warning("The account credentials supplied do not have access to `%s` custom fields.",
stream)
Expand Down Expand Up @@ -158,6 +179,8 @@ def sync(self, state):
self.update_bookmark(state, organization.updated_at)
yield (self.stream, organization)

def check_access(self):
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 comment 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

self.client.organizations.incremental(start_time=START_DATE)

class Users(Stream):
name = "users"
Expand Down Expand Up @@ -235,6 +258,8 @@ def sync(self, state):
start = end - datetime.timedelta(seconds=1)
end = start + datetime.timedelta(seconds=search_window_size)

def check_access(self):
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 a comment 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

self.client.search("", updated_after=START_DATE, updated_before='2000-01-02T00:00:00Z', type="user")

class Tickets(Stream):
name = "tickets"
Expand Down Expand Up @@ -338,6 +363,9 @@ def emit_sub_stream_metrics(sub_stream):
emit_sub_stream_metrics(comments_stream)
singer.write_state(state)

def check_access(self):
self.client.tickets.incremental(start_time=START_DATE, paginate_by_time=False)

class TicketAudits(Stream):
name = "ticket_audits"
replication_method = "INCREMENTAL"
Expand All @@ -349,6 +377,9 @@ def sync(self, ticket_id):
self.count += 1
yield (self.stream, ticket_audit)

def check_access(self):
self.client.tickets.audits(ticket=None)

class TicketMetrics(Stream):
name = "ticket_metrics"
replication_method = "INCREMENTAL"
Expand All @@ -359,6 +390,9 @@ def sync(self, ticket_id):
self.count += 1
yield (self.stream, ticket_metric)

def check_access(self):
self.client.tickets.metrics(ticket=None)

class TicketComments(Stream):
name = "ticket_comments"
replication_method = "INCREMENTAL"
Expand All @@ -370,6 +404,9 @@ def sync(self, ticket_id):
self.count += 1
yield (self.stream, ticket_comment)

def check_access(self):
self.client.tickets.comments(ticket=None)

class SatisfactionRatings(Stream):
name = "satisfaction_ratings"
replication_method = "INCREMENTAL"
Expand Down Expand Up @@ -475,6 +512,9 @@ def sync(self, state):
self.update_bookmark(state, form.updated_at)
yield (self.stream, form)

def check_access(self):
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 a comment 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

self.client.ticket_forms()

class GroupMemberships(Stream):
name = "group_memberships"
replication_method = "INCREMENTAL"
Expand Down Expand Up @@ -512,6 +552,9 @@ def sync(self, state): # pylint: disable=unused-argument
for policy in self.client.sla_policies():
yield (self.stream, policy)

def check_access(self):
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 a comment 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

self.client.sla_policies()

STREAMS = {
"tickets": Tickets,
"groups": Groups,
Expand Down
Loading