TDL-13763: Properly handle 4xx responses for adCampaignGroup stream #28

hpatel41 · 2021-06-29T14:55:51Z

Description of change

TDL-13763: Properly handle 4xx responses for adCampaignGroup stream
TDL-13764: Investigate JSON decoding error

Exception handling is updated with proper custom messages as part of TDL-13763 which will also handle JSON decoding errors.

Manual QA steps

Verified that exception is raised with custom error message as per API error code for JSON decoding error.

Rollback steps

revert this branch

savan-chovatiya · 2021-07-02T06:32:17Z

tap_linkedin_ads/client.py

+    },
+    403: {
+        "raise_exception": LinkedInForbiddenError,
+        "message": "User doesn't have permission to access the resource."


@harshpatel4crest Update the message to "User does not have permission to access the resource."

dbshah1212 · 2021-07-02T11:01:00Z

tests/unittests/test_exception_handling.py

+        except client.LinkedInLengthRequired as e:
+            self.assertEquals(str(e), "HTTP-error-code: 411, Error: {}".format(response_json.get('message')))
+
+    # def test_500_error_response_message(self, mocked_access_token, mocked_request):


Can we remove this?

…into TDL-13763-handle-4xx-error-in-adCampaignGroup

…://github.com/singer-io/tap-linkedin-ads into TDL-13763-handle-4xx-error-in-adCampaignGroup

dbshah1212 · 2021-07-08T12:07:42Z

tap_linkedin_ads/client.py


-class LinkedInForbiddenError(LinkedInError):
+class LinkedInRateLimitExceeeded(Server429Error):


Can we put an Error postfix?

dbshah1212 · 2021-07-08T12:08:12Z

tap_linkedin_ads/client.py

    pass

+class LinkedInLengthRequired(LinkedInError):


Can we put an Error postfix?

dbshah1212 · 2021-07-08T12:08:18Z

tap_linkedin_ads/client.py

@@ -27,65 +26,99 @@ class LinkedInUnauthorizedError(LinkedInError):
    pass


-class LinkedInPaymentRequiredError(LinkedInError):
+class LinkedInMethodNotAllowed(LinkedInError):


Can we put an Error postfix?

dbshah1212 · 2021-07-08T12:10:19Z

tap_linkedin_ads/client.py

+    except Exception:
+        response_json = {}
+
+    if error_code == 404:


If the message is static and we also updating that then do we need to use mapping for 404?

dbshah1212 · 2021-07-08T12:13:08Z

tap_linkedin_ads/client.py

+    else:
+        error_description = response_json.get("message", ERROR_CODE_EXCEPTION_MAPPING.get(error_code, {}).get("message", "Unknown Error") if response_json == {} else response_json)
+
+    if error_code == 400:


Handle this scenario before else condition.

It will handle here only because we are appending the message at the end of response message/custom message.

karanpanchal-crest · 2021-07-19T10:15:58Z

tap_linkedin_ads/client.py

+        response_json = {}
+
+    if error_code == 404:
+        error_description = "The resource you have specified cannot be found."


@harshpatel4crest, Since this, if condition is generic among all streams, please keep the error description in the 'ERROR_CODE_EXCEPTION_MAPPING ' only

karanpanchal-crest · 2021-07-19T10:29:53Z

tap_linkedin_ads/client.py

+        error_description = "The resource you have specified cannot be found."
+        error_description += " Please check the account numbers or you don't have access to the Ad Account."
+    elif response_json.get("message") and "see errorDetails for more information" in response_json.get("message"):
+        error_description = response_json.get("errorDetails").get("inputErrors", [{}])[0].get("code", None)


@harshpatel4crest Please consider the complete information provided in error_description

karanpanchal-crest · 2021-07-20T10:58:16Z

tap_linkedin_ads/client.py

+        error_description = str(response_json.get("errorDetails"))
+    else:
+        # get message from the reponse if present or get custom message if not present
+        error_description = response_json.get("message", ERROR_CODE_EXCEPTION_MAPPING.get(error_code, {}).get("message", "Unknown Error") if response_json == {} else response_json)


@harshpatel4crest Please optimize this condition

karanpanchal-crest · 2021-07-23T10:09:10Z

tap_linkedin_ads/client.py

+    if error_code == 404:
+        # 404 returns "Not Found" so getting custom message
+        error_description = ERROR_CODE_EXCEPTION_MAPPING.get(error_code).get("message")
+    elif response_json.get("errorDetails"):


@harshpatel4crest Code Optimization:
We can remove the elif condition by putting the below code in else condition

response_json.get("errorDetails", response_json.get("message", ERROR_CODE_EXCEPTION_MAPPING.get(error_code, {}).get("message", "Unknown Error")))

KAllan357

This change seems to have lost support for error code 402 - is there a reason why that happened?

hpatel41 · 2021-08-12T06:22:45Z

This change seems to have lost support for error code 402 - is there a reason why that happened?

@KAllan357 We have followed the documentation for the error handling and we haven't found 402 in the (doc) so we removed it.
Let us know if you want to continue adding support for 402.

KAllan357 · 2021-08-12T18:29:45Z

Tests are failing here after merging master.

added code for handling 4xx error

7e06759

hpatel41 requested a review from karanpanchal-crest June 29, 2021 14:55

Added unittest cases for error handling

4daf905

savan-chovatiya reviewed Jul 2, 2021

View reviewed changes

updated error message

61e9803

dbshah1212 reviewed Jul 2, 2021

View reviewed changes

harshpatel4_crest and others added 3 commits July 8, 2021 15:58

Merge branch 'master' of https://github.com/singer-io/tap-linkedin-ads …

a515be9

…into TDL-13763-handle-4xx-error-in-adCampaignGroup

Merge branch 'master' into TDL-13763-handle-4xx-error-in-adCampaignGroup

f8a4b49

Merge branch 'TDL-13763-handle-4xx-error-in-adCampaignGroup' of https…

d71725d

…://github.com/singer-io/tap-linkedin-ads into TDL-13763-handle-4xx-error-in-adCampaignGroup

dbshah1212 suggested changes Jul 8, 2021

View reviewed changes

changed code according to the comments

76d165a

hpatel41 requested a review from dbshah1212 July 9, 2021 10:36

karanpanchal-crest reviewed Jul 19, 2021

View reviewed changes

updated code

6f9b021

karanpanchal-crest reviewed Jul 20, 2021

View reviewed changes

updated the code according to the comments

65f4d70

karanpanchal-crest reviewed Jul 23, 2021

View reviewed changes

removed elif condition

e556b1f

hpatel41 requested review from karanpanchal-crest and savan-chovatiya July 23, 2021 10:47

karanpanchal-crest approved these changes Jul 26, 2021

View reviewed changes

karanpanchal-crest requested review from cosimon and KAllan357 July 26, 2021 05:28

harshpatel4_crest and others added 4 commits July 26, 2021 12:53

updated logger.error message

35b4d4d

updated test cases

05235f2

Merge branch 'master' into TDL-13763-handle-4xx-error-in-adCampaignGroup

b62dc4a

Merge branch 'master' into TDL-13763-handle-4xx-error-in-adCampaignGroup

10efae5

dbshah1212 approved these changes Aug 3, 2021

View reviewed changes

KAllan357 suggested changes Aug 11, 2021

View reviewed changes

hpatel41 and others added 2 commits August 12, 2021 11:52

Merge branch 'master' into TDL-13763-handle-4xx-error-in-adCampaignGroup

f61365b

Merge branch 'master' into TDL-13763-handle-4xx-error-in-adCampaignGroup

f991359

Merge branch 'master' into TDL-13763-handle-4xx-error-in-adCampaignGroup

501dc83

KAllan357 approved these changes Aug 13, 2021

View reviewed changes

KAllan357 merged commit 0f91c27 into master Aug 13, 2021

KAllan357 deleted the TDL-13763-handle-4xx-error-in-adCampaignGroup branch August 13, 2021 14:57

KrisPersonal mentioned this pull request Jan 10, 2022

Bump version for few changes #37

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TDL-13763: Properly handle 4xx responses for adCampaignGroup stream #28

TDL-13763: Properly handle 4xx responses for adCampaignGroup stream #28

hpatel41 commented Jun 29, 2021 •

edited by savan-chovatiya

Loading

savan-chovatiya Jul 2, 2021

hpatel41 Jul 2, 2021

dbshah1212 Jul 2, 2021

dbshah1212 Jul 8, 2021

hpatel41 Jul 9, 2021

dbshah1212 Jul 8, 2021

hpatel41 Jul 9, 2021

dbshah1212 Jul 8, 2021

hpatel41 Jul 9, 2021

dbshah1212 Jul 8, 2021

hpatel41 Jul 9, 2021

dbshah1212 Jul 8, 2021

hpatel41 Jul 9, 2021

karanpanchal-crest Jul 19, 2021 •

edited

Loading

karanpanchal-crest Jul 19, 2021

karanpanchal-crest Jul 20, 2021

karanpanchal-crest Jul 23, 2021

hpatel41 Jul 30, 2021

KAllan357 left a comment

hpatel41 commented Aug 12, 2021 •

edited

Loading

KAllan357 commented Aug 12, 2021


		class LinkedInForbiddenError(LinkedInError):
		class LinkedInRateLimitExceeeded(Server429Error):

TDL-13763: Properly handle 4xx responses for adCampaignGroup stream #28

TDL-13763: Properly handle 4xx responses for adCampaignGroup stream #28

Conversation

hpatel41 commented Jun 29, 2021 • edited by savan-chovatiya Loading

Description of change

Manual QA steps

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

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

karanpanchal-crest Jul 19, 2021 • 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

KAllan357 left a comment

Choose a reason for hiding this comment

hpatel41 commented Aug 12, 2021 • edited Loading

KAllan357 commented Aug 12, 2021

hpatel41 commented Jun 29, 2021 •

edited by savan-chovatiya

Loading

karanpanchal-crest Jul 19, 2021 •

edited

Loading

hpatel41 commented Aug 12, 2021 •

edited

Loading