feat(airbyte-cdk): Have better fallback error message #43399
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
| return f"Unexpected response with HTTP status {response.status_code}" | ||
|
|
||
|
|
||
| def create_fallback_error_resolution(response_or_exception: Union[requests.Response, Exception]) -> ErrorResolution: |
There was a problem hiding this comment.
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(
) does not seem to passNone.
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
There was a problem hiding this comment.
Until we figure this out, this will generate mypy issues as seen here
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
airbyte-cdk/python/airbyte_cdk/sources/streams/http/error_handlers/response_models.py
Show resolved
Hide resolved
airbyte-cdk/python/airbyte_cdk/sources/streams/http/error_handlers/response_models.py
Outdated
Show resolved
Hide resolved
|
/approve-regression-tests
|
What
Addresses https://github.com/airbytehq/airbyte-internal-issues/issues/9024
When there is an unexpected error, the error message is just
The request failed due to an unknown error.We need better messages.How
Replace DEFAULT_ERROR_RESOLUTION with a function that extracts information from the exception or the response.
Review guide
airbyte-cdk/python/airbyte_cdk/sources/streams/http/error_handlers/response_models.pyUser Impact
On HTTP errors that the CDK does not handle, the error should have more information to debug
Can this PR be safely reverted and rolled back?