-
Notifications
You must be signed in to change notification settings - Fork 84
CUST-4514 added handling for non-json responses and tests #424
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
base: main
Are you sure you want to change the base?
Changes from 6 commits
e691cd2
ebedcce
2a85633
445901e
2a604c9
da421ad
338844e
24ca286
1bfc7d7
eff978a
58da633
8f0f893
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 |
|---|---|---|
|
|
@@ -18,7 +18,22 @@ | |
|
|
||
|
|
||
| def _validate_response(response: Response) -> Tuple[Dict, CaseInsensitiveDict]: | ||
| json = response.json() | ||
| try: | ||
| json = response.json() | ||
| except ValueError as exc: | ||
| if response.status_code >= 400: | ||
| raise NylasApiError( | ||
| NylasApiErrorResponse( | ||
| None, | ||
| NylasApiErrorResponseData( | ||
| type="network_error", | ||
| message=f"HTTP {response.status_code}: Non-JSON response received", | ||
| ), | ||
| ), | ||
| status_code=response.status_code, | ||
| headers=response.headers, | ||
| ) from exc | ||
| return ({}, response.headers) | ||
|
Contributor
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. Silently returning |
||
| if response.status_code >= 400: | ||
| parsed_url = urlparse(response.url) | ||
| try: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,9 +28,9 @@ def __init__( | |
| status_code: The HTTP status code of the error response. | ||
| message: The error message. | ||
| """ | ||
| self.request_id: str = request_id | ||
| self.status_code: int = status_code | ||
| self.headers: CaseInsensitiveDict = headers | ||
| self.request_id: Optional[str] = request_id | ||
| self.status_code: Optional[int] = status_code | ||
| self.headers: Optional[CaseInsensitiveDict] = headers | ||
|
Contributor
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. Per the earlier review thread — these shouldn't become Optional on the base class. Genuine API errors always have a |
||
| super().__init__(message) | ||
|
|
||
|
|
||
|
|
@@ -70,7 +70,7 @@ class NylasApiErrorResponse: | |
| error: The error data. | ||
| """ | ||
|
|
||
| request_id: str | ||
| request_id: Optional[str] | ||
|
Contributor
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. I don't believe this is ever optional for our API effors
Contributor
Author
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. Thanks, I'll correct this! |
||
| error: NylasApiErrorResponseData | ||
|
|
||
|
|
||
|
|
@@ -169,3 +169,47 @@ def __init__(self, url: str, timeout: int, headers: Optional[CaseInsensitiveDict | |
| self.url: str = url | ||
| self.timeout: int = timeout | ||
| self.headers: CaseInsensitiveDict = headers | ||
|
|
||
|
|
||
| class NylasNetworkError(AbstractNylasSdkError): | ||
|
Contributor
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. Where are we actually utilizing this new error?
Contributor
Author
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. Nowhere yet, the idea is that it will replace the proposed non-json handling implementation in the future, we currently raise a NylasApiError to keep backwards compatibility as a priority, let me know if we should just use this instead. |
||
| """ | ||
| Error thrown when the SDK receives a non-JSON response with an error status code. | ||
| This typically happens when the request never reaches the Nylas API due to | ||
| infrastructure issues (e.g., proxy errors, load balancer failures). | ||
|
|
||
| Note: This error class will be used in v7.0 to replace NylasApiError for non-JSON | ||
| HTTP error responses. Currently, non-JSON errors still throw NylasApiError with | ||
| type="network_error" for backwards compatibility. | ||
|
|
||
| Attributes: | ||
| request_id: The unique identifier of the request. | ||
| status_code: The HTTP status code of the error response. | ||
| raw_body: The non-JSON response body. | ||
| headers: The headers returned from the server. | ||
| flow_id: The value from x-fastly-id header if present. | ||
| """ | ||
|
|
||
| def __init__( | ||
| self, | ||
| message: str, | ||
| request_id: Optional[str] = None, | ||
| status_code: Optional[int] = None, | ||
| raw_body: Optional[str] = None, | ||
| headers: Optional[CaseInsensitiveDict] = None, | ||
| flow_id: Optional[str] = None, | ||
| ): | ||
| """ | ||
| Args: | ||
| message: The error message. | ||
| request_id: The unique identifier of the request. | ||
| status_code: The HTTP status code of the error response. | ||
| raw_body: The non-JSON response body. | ||
| headers: The headers returned from the server. | ||
| flow_id: The value from x-fastly-id header if present. | ||
| """ | ||
| super().__init__(message) | ||
| self.request_id: Optional[str] = request_id | ||
| self.status_code: Optional[int] = status_code | ||
| self.raw_body: Optional[str] = raw_body | ||
| self.headers: Optional[CaseInsensitiveDict] = headers | ||
| self.flow_id: Optional[str] = flow_id | ||
|
Contributor
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. This class is defined but never instantiated. Either raise it from |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -432,3 +432,65 @@ def test_execute_with_headers(self, http_client, patched_version_and_sys, patche | |
| timeout=30, | ||
| data=None, | ||
| ) | ||
|
|
||
| def test_validate_response_500_error_html(self): | ||
| response = Mock() | ||
| response.status_code = 500 | ||
| response.json.side_effect = ValueError("No JSON object could be decoded") | ||
| response.text = "<html><body><h1>Internal Server Error</h1></body></html>" | ||
| response.headers = {"Content-Type": "text/html", "x-fastly-id": "fastly-123"} | ||
|
|
||
| with pytest.raises(NylasApiError) as e: | ||
| _validate_response(response) | ||
|
Contributor
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. These assert "HTTP 500" in str(e.value)
assert "flow_id: fastly-123" in str(e.value)
assert "<h1>Internal Server Error</h1>" in str(e.value) |
||
| assert e.value.type == "network_error" | ||
| assert str(e.value) == "HTTP 500: Non-JSON response received" | ||
| assert e.value.status_code == 500 | ||
|
|
||
| def test_validate_response_502_error_plain_text(self): | ||
| response = Mock() | ||
| response.status_code = 502 | ||
| response.json.side_effect = ValueError("No JSON object could be decoded") | ||
| response.text = "Bad Gateway" | ||
| response.headers = {"Content-Type": "text/plain"} | ||
|
|
||
| with pytest.raises(NylasApiError) as e: | ||
| _validate_response(response) | ||
| assert e.value.type == "network_error" | ||
| assert str(e.value) == "HTTP 502: Non-JSON response received" | ||
| assert e.value.status_code == 502 | ||
|
|
||
| def test_validate_response_200_success_non_json(self): | ||
| response = Mock() | ||
| response.status_code = 200 | ||
| response.json.side_effect = ValueError("No JSON object could be decoded") | ||
| response.headers = {"Content-Type": "text/plain"} | ||
|
|
||
| response_json, response_headers = _validate_response(response) | ||
| assert response_json == {} | ||
| assert response_headers == {"Content-Type": "text/plain"} | ||
|
|
||
| def test_validate_response_error_empty_response(self): | ||
| response = Mock() | ||
| response.status_code = 500 | ||
| response.json.side_effect = ValueError("No JSON object could be decoded") | ||
| response.text = "" | ||
| response.headers = {"Content-Type": "text/html"} | ||
|
|
||
| with pytest.raises(NylasApiError) as e: | ||
| _validate_response(response) | ||
| assert e.value.type == "network_error" | ||
| assert str(e.value) == "HTTP 500: Non-JSON response received" | ||
| assert e.value.status_code == 500 | ||
|
|
||
| def test_validate_response_error_long_response_not_truncated(self): | ||
| response = Mock() | ||
| response.status_code = 500 | ||
| response.json.side_effect = ValueError("No JSON object could be decoded") | ||
| response.text = "A" * 600 | ||
| response.headers = {"Content-Type": "text/html"} | ||
|
|
||
| with pytest.raises(NylasApiError) as e: | ||
| _validate_response(response) | ||
| assert e.value.type == "network_error" | ||
| assert str(e.value) == "HTTP 500: Non-JSON response received" | ||
| assert e.value.status_code == 500 | ||
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.
This entry is under
v6.10.0, which has already been released. Please move it under anUnreleasedsection at the top so it lands in the next release notes.