TDL-17516: Add custom exception handling #8

hpatel41 · 2022-02-02T05:18:05Z

Description of change

TDL-17516: Add custom exception handling

Added error handling based on the document

NOTE: the build is failing due to integration tests failure but that is fixed and all tests are written as part of #5

Manual QA steps

Risks

Rollback steps

revert this branch

savan-chovatiya · 2022-02-02T06:17:58Z

tap_sailthru/client.py

+    },
+    404: {
+        "raise_exception": SailthruNotFoundError,
+        "message": "The resource you have specified cannot be found. Either the accounts provided are invalid or you do not have access to the Ad Account."


Suggested change

"message": "The resource you have specified cannot be found. Either the accounts provided are invalid or you do not have access to the Ad Account."

"message": "The resource you have specified cannot be found."

Updated error message.

savan-chovatiya · 2022-02-02T06:59:50Z

tap_sailthru/client.py

+    if status_code == 403 and error_code == 99:
+        LOGGER.warning("{}".format(json_response))
+


The tap was not raising exceptions for the above specific condition earlier so update this to not raise expectation for this particular scenario.

tap-sailthru/tap_sailthru/client.py

Lines 237 to 240 in 249d3ae

if response.status_code == 403 and response.json().get("error") == 99:

# pylint: disable=logging-fstring-interpolation

LOGGER.warning(f"{response.json()}")

return response.json()

As this is an error, we should raise it instead of returning the response. As the Tap is expecting data from the response, it may break at another location.

dbshah1212 · 2022-02-02T10:54:03Z

tap_sailthru/client.py

+        "message": "API rate limit exceeded, please retry after some time."
+    },
+    500: {
+        "raise_exception": SailthruServer5xxError,


Do define 5xx and 500 differently. Make 500 a sub-class of 5xx because it is an internal server error specifically.

dbshah1212 · 2022-02-02T10:55:08Z

tap_sailthru/client.py

+def get_exception_for_status_code(status_code, error_code):
+    if status_code >= 500:
+        return SailthruServer5xxError
+    if status_code == 400 and error_code == 99:


Please add comments for this specific scenario.

This condition was already present, we have just changed the location.

Still better to add the comment for future developers.

We did not find any doc regarding this, not sure why it was implemented before.

dbshah1212 · 2022-02-02T10:57:01Z

tap_sailthru/client.py

+
+    return ERROR_CODE_EXCEPTION_MAPPING.get(status_code, {}).get("raise_exception", SailthruClientError)
+
+def raise_for_error(response):


Provide function comments for all functions.

dbshah1212 · 2022-02-02T10:59:09Z

tests/unittests/test_error_handling.py

@@ -0,0 +1,253 @@
+from unittest import mock


Do provide test function comment

* TDL-17511-Add missing test case, TDL-17522-Fix typos in the fields (#5) * Added all the integration test cases * Added one assertion in discovery test * used pytest with coverage * removed un-used line from config.yml file * reverted the change * updated config.yml file for unittest coverage * removed user_agent from base test file * skip blast_query from start date and bookmark tests * removed unsed imports and updated state bookmark in bookmark test * removed setuptools * resolved bookmark test failure * resolved bookmark test failure * include blast_query stream in test cases Co-authored-by: harshpatel4_crest <[email protected]> * TDL-17521: Fix date_windowing for the purchase_log stream (#9) * Added all the integration test cases * updated date window for purchase log stream * run purchase log stream in start date test * Added one assertion in discovery test * used pytest with coverage * removed un-used line from config.yml file * reverted the change * updated config.yml file for unittest coverage * removed user_agent from base test file * skip blast_query from start date and bookmark tests Co-authored-by: Umang Agrawal <[email protected]> * TDL-17514: Implement request timeout (#6) * Added request timeout * Resolved review comments * run CCi build * updated config,yml file for unittest reports * removed user_agent and imported tap tester libs in base test file Co-authored-by: harshpatel4_crest <[email protected]> * TDL-17515: Verify credentials in discover mode (#7) * Added credentials check in discover mode * updated unittest and run CCi * removed user_agent from base file test * imported runner in base test file * imported menagerie in base test file Co-authored-by: harshpatel4_crest <[email protected]> * TDL-17516: Add custom exception handling (#8) * added exception handling * updated 404 error message * added 500 error class and comments * run CCi build and updated unittest assertion * updated config.yml file * updated config.yml file * removed user_agent from base test file * imported tap tester libs in base * TDL-17520: Primary Key is not unique for the purchase_log stream (#10) * updated primary key for purchase_log stream * removed user_agent from base test file * imported tap_tester in base test file * added channel in the primary key for purchase log * resolve pylint error * resolve unittest failure * skip 403 and 99 error, updated automatic test case for purchase_log stream Co-authored-by: Umang Agrawal <[email protected]> Co-authored-by: Umang Agrawal <[email protected]> Co-authored-by: savan-chovatiya <[email protected]>

added exception handling

c69f210

savan-chovatiya marked this pull request as ready for review February 2, 2022 07:00

savan-chovatiya requested review from savan-chovatiya, dbshah1212 and karanpanchal-crest February 2, 2022 07:00

savan-chovatiya suggested changes Feb 2, 2022

View reviewed changes

updated 404 error message

3017b57

hpatel41 requested a review from savan-chovatiya February 2, 2022 09:08

dbshah1212 reviewed Feb 2, 2022

View reviewed changes

added 500 error class and comments

77bc70f

hpatel41 requested a review from dbshah1212 February 2, 2022 11:51

karanpanchal-crest approved these changes Feb 4, 2022

View reviewed changes

harshpatel4_crest added 5 commits February 7, 2022 15:25

run CCi build and updated unittest assertion

5a7a34f

updated config.yml file

f908c8c

updated config.yml file

f357ed5

removed user_agent from base test file

cea4ba5

imported tap tester libs in base

6196c31

hpatel41 requested review from KrisPersonal, kspeer825 and manand31 February 9, 2022 08:18

KrisPersonal approved these changes Feb 11, 2022

View reviewed changes

kspeer825 approved these changes Feb 14, 2022

View reviewed changes

resolve merge conflicts

fd96c7d

hpatel41 merged commit 9a46e85 into crest-work Feb 24, 2022

hpatel41 mentioned this pull request Feb 24, 2022

Crest Work #11

Merged

KrisPersonal mentioned this pull request Mar 2, 2022

Beta Version 0.2.0 #12

Merged

somethingmorerelevant deleted the TDL-17516-add-exception-handling branch February 15, 2023 14:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TDL-17516: Add custom exception handling #8

TDL-17516: Add custom exception handling #8

hpatel41 commented Feb 2, 2022 •

edited

Loading

savan-chovatiya Feb 2, 2022

hpatel41 Feb 2, 2022

savan-chovatiya Feb 2, 2022

hpatel41 Feb 2, 2022

dbshah1212 Feb 2, 2022

hpatel41 Feb 2, 2022

dbshah1212 Feb 2, 2022

hpatel41 Feb 2, 2022 •

edited

Loading

dbshah1212 Feb 2, 2022

hpatel41 Feb 7, 2022

dbshah1212 Feb 2, 2022

hpatel41 Feb 2, 2022

dbshah1212 Feb 2, 2022

hpatel41 Feb 2, 2022

	"message": "The resource you have specified cannot be found. Either the accounts provided are invalid or you do not have access to the Ad Account."
	"message": "The resource you have specified cannot be found."

		if status_code == 403 and error_code == 99:
		LOGGER.warning("{}".format(json_response))

	if response.status_code == 403 and response.json().get("error") == 99:
	# pylint: disable=logging-fstring-interpolation
	LOGGER.warning(f"{response.json()}")
	return response.json()


		return ERROR_CODE_EXCEPTION_MAPPING.get(status_code, {}).get("raise_exception", SailthruClientError)

		def raise_for_error(response):

TDL-17516: Add custom exception handling #8

TDL-17516: Add custom exception handling #8

Conversation

hpatel41 commented Feb 2, 2022 • edited Loading

Description of change

Manual QA steps

Risks

Rollback steps

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hpatel41 Feb 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hpatel41 commented Feb 2, 2022 •

edited

Loading

hpatel41 Feb 2, 2022 •

edited

Loading