Skip to content

feat(airbyte-cdk): Have better fallback error message #43399

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

Merged
merged 5 commits into from
Aug 13, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@

import requests
from airbyte_cdk.sources.streams.http.error_handlers import ErrorHandler
from airbyte_cdk.sources.streams.http.error_handlers.response_models import DEFAULT_ERROR_RESOLUTION, ErrorResolution, ResponseAction
from airbyte_cdk.sources.streams.http.error_handlers.response_models import ErrorResolution, ResponseAction, create_fallback_error_resolution


@dataclass
Expand Down Expand Up @@ -69,4 +69,4 @@ def interpret_response(self, response_or_exception: Optional[Union[requests.Resp
if matched_error_resolution:
return matched_error_resolution

return DEFAULT_ERROR_RESOLUTION
return create_fallback_error_resolution(response_or_exception)
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
from airbyte_cdk.sources.declarative.requesters.error_handlers.default_http_response_filter import DefaultHttpResponseFilter
from airbyte_cdk.sources.declarative.requesters.error_handlers.http_response_filter import HttpResponseFilter
from airbyte_cdk.sources.streams.http.error_handlers import BackoffStrategy, ErrorHandler
from airbyte_cdk.sources.streams.http.error_handlers.response_models import DEFAULT_ERROR_RESOLUTION, SUCCESS_RESOLUTION, ErrorResolution
from airbyte_cdk.sources.streams.http.error_handlers.response_models import SUCCESS_RESOLUTION, ErrorResolution, create_fallback_error_resolution
from airbyte_cdk.sources.types import Config


Expand Down Expand Up @@ -114,7 +114,7 @@ def interpret_response(self, response_or_exception: Optional[Union[requests.Resp
default_reponse_filter = DefaultHttpResponseFilter(parameters={}, config=self.config)
default_response_filter_resolution = default_reponse_filter.matches(response_or_exception)

return default_response_filter_resolution if default_response_filter_resolution else DEFAULT_ERROR_RESOLUTION
return default_response_filter_resolution if default_response_filter_resolution else create_fallback_error_resolution(response_or_exception)

def backoff_time(
self, response_or_exception: Optional[Union[requests.Response, requests.RequestException]], attempt_count: int = 0
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
import requests
from airbyte_cdk.sources.declarative.requesters.error_handlers.http_response_filter import HttpResponseFilter
from airbyte_cdk.sources.streams.http.error_handlers.default_error_mapping import DEFAULT_ERROR_MAPPING
from airbyte_cdk.sources.streams.http.error_handlers.response_models import DEFAULT_ERROR_RESOLUTION, ErrorResolution
from airbyte_cdk.sources.streams.http.error_handlers.response_models import ErrorResolution, create_fallback_error_resolution


class DefaultHttpResponseFilter(HttpResponseFilter):
Expand All @@ -25,4 +25,4 @@ def matches(self, response_or_exception: Optional[Union[requests.Response, Excep

default_mapped_error_resolution = DEFAULT_ERROR_MAPPING.get(mapped_key)

return default_mapped_error_resolution if default_mapped_error_resolution else DEFAULT_ERROR_RESOLUTION
return default_mapped_error_resolution if default_mapped_error_resolution else create_fallback_error_resolution(response_or_exception)
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,13 @@

from dataclasses import dataclass
from enum import Enum
from typing import Optional
from typing import Optional, Union

import requests
from requests import HTTPError

from airbyte_cdk.models import FailureType
from airbyte_cdk.utils.airbyte_secrets_utils import filter_secrets


class ResponseAction(Enum):
Expand All @@ -22,10 +26,27 @@ class ErrorResolution:
error_message: Optional[str] = None


DEFAULT_ERROR_RESOLUTION = ErrorResolution(
response_action=ResponseAction.RETRY,
failure_type=FailureType.system_error,
error_message="The request failed due to an unknown error.",
)
def _format_exception_error_message(exception: Exception) -> str:
return f"{type(exception).__name__}: {str(exception)}"


def _format_response_error_message(response: requests.Response) -> str:
try:
response.raise_for_status()
except HTTPError as exception:
return filter_secrets(f"Response was not ok: `{str(exception)}`. Response content is: {response.text}")
# We purposefully do not add the response.content because the response is "ok" so there might be sensitive information in the payload.
# Feel free the
return f"Unexpected response with HTTP status {response.status_code}"


def create_fallback_error_resolution(response_or_exception: Union[requests.Response, Exception]) -> ErrorResolution:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that the interface does not accept None like the interpret_response does. I started a conversation in our private slack. Basically:

In which case can response_or_exception be None? The typing says that it is optional but the only call where response_or_exception is not a passthrough(

error_resolution: ErrorResolution = self._error_handler.interpret_response(response if response is not None else exc)
) does not seem to pass None.

I’m asking because:

  • I’m not sure what it would mean
  • It’s another case to handle when dealing with the parameter response_or_exception
  • Most error handlers do not seem to handle which seems dangerous for AttributeError: 'NoneType' object has no attribute X

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Until we figure this out, this will generate mypy issues as seen here

Copy link
Contributor

Choose a reason for hiding this comment

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

Based on the Slack thread, is the proposal to modify the interpret_response() method interface to no longer be Optional[blahblah]? And that would be the solve for typing isses since interpret_response() -> create_fallback_error_resolution() will be passing the union now.

I'm not opposed to that, but are worried about all the backwards compatibility headaches? since we do have some uses of the various DefaultErrorHandlers that technically still adhere to the same interface?

Would the alternative to the mypy fix be to add a type check or None check around the calls to create_fallback_error_resolution() to guarantee it's not None, even though we're confident it never is. I didn't test myself, but I'm curious if that would make mypy stop complaining

Or the alternative alternative is just an ignore lol

Copy link
Contributor Author

@maxi297 maxi297 Aug 12, 2024

Choose a reason for hiding this comment

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

Agreed that it is a bigger change. Let's decouple the typing change from this change then. I'll just have the error message be something like "Unexpected argument while handling error." and add a comment that we expect this not to happen

error_message = _format_exception_error_message(response_or_exception) if isinstance(response_or_exception, Exception) else _format_response_error_message(response_or_exception)
return ErrorResolution(
response_action=ResponseAction.RETRY,
failure_type=FailureType.system_error,
error_message=error_message,
)


SUCCESS_RESOLUTION = ErrorResolution(response_action=ResponseAction.SUCCESS, failure_type=None, error_message=None)
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@

import pytest
import requests
from airbyte_protocol.models import FailureType

from airbyte_cdk.sources.declarative.requesters.error_handlers import HttpResponseFilter
from airbyte_cdk.sources.declarative.requesters.error_handlers.composite_error_handler import CompositeErrorHandler
from airbyte_cdk.sources.declarative.requesters.error_handlers.default_error_handler import DefaultErrorHandler
Expand Down Expand Up @@ -97,6 +99,24 @@ def test_composite_error_handler(test_name, first_handler_behavior, second_handl
assert retrier.interpret_response(response_mock) == expected_behavior


def test_given_unmatched_response_or_exception_then_return_default_error_resolution():
composite_error_handler = CompositeErrorHandler(
error_handlers=[
DefaultErrorHandler(
response_filters=[],
parameters={},
config={},
)
],
parameters={},
)

error_resolution = composite_error_handler.interpret_response(ValueError("Any error"))

assert error_resolution.response_action == ResponseAction.RETRY
assert error_resolution.failure_type == FailureType.system_error


def test_composite_error_handler_no_handlers():
try:
CompositeErrorHandler(error_handlers=[], parameters={})
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,7 @@
)
from airbyte_cdk.sources.declarative.requesters.error_handlers.default_error_handler import DefaultErrorHandler, HttpResponseFilter
from airbyte_cdk.sources.streams.http.error_handlers.default_error_mapping import DEFAULT_ERROR_MAPPING
from airbyte_cdk.sources.streams.http.error_handlers.response_models import (
DEFAULT_ERROR_RESOLUTION,
ErrorResolution,
FailureType,
ResponseAction,
)
from airbyte_cdk.sources.streams.http.error_handlers.response_models import ErrorResolution, FailureType, ResponseAction

SOME_BACKOFF_TIME = 60

Expand Down Expand Up @@ -55,7 +50,7 @@
ErrorResolution(
response_action=ResponseAction.RETRY,
failure_type=FailureType.system_error,
error_message="The request failed due to an unknown error.",
error_message="Unexpected response with HTTP status 418",
),
)
],
Expand Down Expand Up @@ -246,7 +241,9 @@ def test_default_error_handler_with_unmapped_http_code():
response_mock.ok = False
response_mock.headers = {}
actual_error_resolution = error_handler.interpret_response(response_mock)
assert actual_error_resolution == DEFAULT_ERROR_RESOLUTION
assert actual_error_resolution
assert actual_error_resolution.failure_type == FailureType.system_error
assert actual_error_resolution.response_action == ResponseAction.RETRY


def test_predicate_takes_precedent_over_default_mapped_error():
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,11 @@
from unittest.mock import MagicMock

import pytest
from airbyte_protocol.models import FailureType

from airbyte_cdk.sources.declarative.requesters.error_handlers.default_http_response_filter import DefaultHttpResponseFilter
from airbyte_cdk.sources.streams.http.error_handlers.default_error_mapping import DEFAULT_ERROR_MAPPING
from airbyte_cdk.sources.streams.http.error_handlers.response_models import DEFAULT_ERROR_RESOLUTION
from airbyte_cdk.sources.streams.http.error_handlers.response_models import ResponseAction
from requests import RequestException, Response


Expand Down Expand Up @@ -69,4 +71,6 @@ def test_unmapped_http_status_code_returns_default_error_resolution():
)

actual_error_resolution = response_filter.matches(response)
assert actual_error_resolution == DEFAULT_ERROR_RESOLUTION
assert actual_error_resolution
assert actual_error_resolution.failure_type == FailureType.system_error
assert actual_error_resolution.response_action == ResponseAction.RETRY
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
# Copyright (c) 2024 Airbyte, Inc., all rights reserved.

from unittest import TestCase

import requests
import requests_mock
from airbyte_protocol.models import FailureType

from airbyte_cdk.sources.streams.http.error_handlers.response_models import create_fallback_error_resolution, ResponseAction
from airbyte_cdk.utils.airbyte_secrets_utils import update_secrets


_A_SECRET = "a-secret"
_A_URL = "https://a-url.com"

class DefaultErrorResolutionTest(TestCase):

def setUp(self) -> None:
update_secrets([_A_SECRET])

def tearDown(self) -> None:
# to avoid other tests being impacted by added secrets
update_secrets([])

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

def _create_response(self, status_code: int) -> requests.Response:
with requests_mock.Mocker() as http_mocker:
http_mocker.get(_A_URL, status_code=status_code)
return requests.get(_A_URL)
Loading