Skip to content

poc: ErrorHandler.interpret_response can return None #790

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

poc: ErrorHandler.interpret_response can return None #790

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 0 additions & 3 deletions airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2277,9 +2277,6 @@ def create_default_error_handler(
response_filters.append(
self._create_component_from_model(model=response_filter_model, config=config)
)
response_filters.append(
HttpResponseFilter(config=config, parameters=model.parameters or {})
)
Copy link
Contributor

Choose a reason for hiding this comment

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

why was this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I assume because the fallback is create_fallback_error_resolution which was returning a system_error with RETRY


return DefaultErrorHandler(
backoff_strategies=backoff_strategies,
Expand Down
16 changes: 2 additions & 14 deletions airbyte_cdk/sources/declarative/requesters/error_handlers/composite_error_handler.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,25 +58,13 @@ def max_time(self) -> Optional[int]:

def interpret_response(
self, response_or_exception: Optional[Union[requests.Response, Exception]]
) -> ErrorResolution:
matched_error_resolution = None
) -> Optional[ErrorResolution]:
for error_handler in self.error_handlers:
matched_error_resolution = error_handler.interpret_response(response_or_exception)

if not isinstance(matched_error_resolution, ErrorResolution):
continue

if matched_error_resolution.response_action in [
ResponseAction.SUCCESS,
ResponseAction.RETRY,
ResponseAction.IGNORE,
ResponseAction.RESET_PAGINATION,
]:
if isinstance(matched_error_resolution, ErrorResolution):
return matched_error_resolution

if matched_error_resolution:
return matched_error_resolution

return create_fallback_error_resolution(response_or_exception)

@property
Expand Down
4 changes: 2 additions & 2 deletions airbyte_cdk/sources/declarative/requesters/error_handlers/default_error_handler.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,8 +122,8 @@ def interpret_response(
if response_or_exception.ok:
return SUCCESS_RESOLUTION

default_reponse_filter = DefaultHttpResponseFilter(parameters={}, config=self.config)
default_response_filter_resolution = default_reponse_filter.matches(response_or_exception)
default_response_filter = DefaultHttpResponseFilter(parameters={}, config=self.config)
default_response_filter_resolution = default_response_filter.matches(response_or_exception)

return (
default_response_filter_resolution
Expand Down
2 changes: 1 addition & 1 deletion airbyte_cdk/sources/streams/http/error_handlers/error_handler.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ def max_time(self) -> Optional[int]:
@abstractmethod
def interpret_response(
self, response: Optional[Union[requests.Response, Exception]]
) -> ErrorResolution:
) -> Optional[ErrorResolution]:
"""
Interpret the response or exception and return the corresponding response action, failure type, and error message.

Expand Down
2 changes: 1 addition & 1 deletion airbyte_cdk/sources/streams/http/error_handlers/http_status_error_handler.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ def max_time(self) -> Optional[int]:

def interpret_response(
self, response_or_exception: Optional[Union[requests.Response, Exception]] = None
) -> ErrorResolution:
) -> Optional[ErrorResolution]:
"""
Interpret the response and return the corresponding response action, failure type, and error message.

Expand Down
16 changes: 2 additions & 14 deletions airbyte_cdk/sources/streams/http/error_handlers/response_models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,20 +45,8 @@ def _format_response_error_message(response: requests.Response) -> str:

def create_fallback_error_resolution(
response_or_exception: Optional[Union[requests.Response, Exception]],
) -> ErrorResolution:
if response_or_exception is None:
# We do not expect this case to happen but if it does, it would be good to understand the cause and improve the error message
error_message = "Error handler did not receive a valid response or exception. This is unexpected please contact Airbyte Support"
elif isinstance(response_or_exception, Exception):
error_message = _format_exception_error_message(response_or_exception)
else:
error_message = _format_response_error_message(response_or_exception)

return ErrorResolution(
response_action=ResponseAction.RETRY,
failure_type=FailureType.system_error,
error_message=error_message,
)
) -> None:
return None


SUCCESS_RESOLUTION = ErrorResolution(
Expand Down
2 changes: 1 addition & 1 deletion airbyte_cdk/sources/streams/http/http.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -628,7 +628,7 @@ def __init__(self, stream: HttpStream, **kwargs): # type: ignore # noqa

def interpret_response(
self, response_or_exception: Optional[Union[requests.Response, Exception]] = None
) -> ErrorResolution:
) -> Optional[ErrorResolution]:
if isinstance(response_or_exception, Exception):
return super().interpret_response(response_or_exception)
elif isinstance(response_or_exception, requests.Response):
Expand Down
4 changes: 2 additions & 2 deletions airbyte_cdk/sources/streams/http/http_client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -342,7 +342,7 @@ def _send(
except requests.RequestException as e:
exc = e

error_resolution: ErrorResolution = self._error_handler.interpret_response(
error_resolution: Optional[ErrorResolution] = self._error_handler.interpret_response(
response if response is not None else exc
)

Expand Down Expand Up @@ -380,7 +380,7 @@ def _send(
response=response,
exc=exc,
request=request,
error_resolution=error_resolution,
error_resolution=error_resolution if error_resolution else ErrorResolution(ResponseAction.SUCCESS),
exit_on_rate_limit=exit_on_rate_limit,
)

Expand Down
53 changes: 3 additions & 50 deletions unit_tests/sources/streams/http/error_handlers/test_response_models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
# Copyright (c) 2024 Airbyte, Inc., all rights reserved.

from unittest import TestCase
from unittest.mock import Mock

import requests
import requests_mock
Expand All @@ -27,56 +28,8 @@ def tearDown(self) -> None:
def test_given_none_when_create_fallback_error_resolution_then_return_error_resolution(
self,
) -> None:
error_resolution = create_fallback_error_resolution(None)

assert error_resolution.failure_type == FailureType.system_error
assert error_resolution.response_action == ResponseAction.RETRY
assert (
error_resolution.error_message
== "Error handler did not receive a valid response or exception. This is unexpected please contact Airbyte Support"
)

def test_given_exception_when_create_fallback_error_resolution_then_return_error_resolution(
self,
) -> None:
exception = ValueError("This is an exception")

error_resolution = create_fallback_error_resolution(exception)

assert error_resolution.failure_type == FailureType.system_error
assert error_resolution.response_action == ResponseAction.RETRY
assert error_resolution.error_message
assert "ValueError" in error_resolution.error_message
assert str(exception) in error_resolution.error_message

def test_given_response_can_raise_for_status_when_create_fallback_error_resolution_then_error_resolution(
self,
) -> None:
response = self._create_response(512)

error_resolution = create_fallback_error_resolution(response)

assert error_resolution.failure_type == FailureType.system_error
assert error_resolution.response_action == ResponseAction.RETRY
assert (
error_resolution.error_message
and "512 Server Error: None for url: https://a-url.com/"
in error_resolution.error_message
)

def test_given_response_is_ok_when_create_fallback_error_resolution_then_error_resolution(
self,
) -> None:
response = self._create_response(205)

error_resolution = create_fallback_error_resolution(response)

assert error_resolution.failure_type == FailureType.system_error
assert error_resolution.response_action == ResponseAction.RETRY
assert (
error_resolution.error_message
and str(response.status_code) in error_resolution.error_message
)
any_response_or_exception = Mock()
assert create_fallback_error_resolution(any_response_or_exception) is None

def _create_response(self, status_code: int) -> requests.Response:
with requests_mock.Mocker() as http_mocker:
Expand Down
Loading