update SaasConnector.send behavior on ignore_errors to return raw response #462
Conversation
…errors. move response conversion into higher-level handling method
| if saas_request.ignore_errors and not response.ok: | ||
| logger.info( | ||
| f"Ignoring and clearing errored response with status code {response.status_code}." | ||
| ) | ||
| response = Response() | ||
| response._content = b"{}" # pylint: disable=W0212 | ||
|
|
There was a problem hiding this comment.
execute_prepared_request is getting pretty long now, and the behavior of replacing an errored response with an empty dictionary is not protected by a unit test, just an end-to-end sentry integration test.
I'd consider pulling this out into its own function then, the code comment can be the function docstring, and then a fake SaasRequest and Response can be created for unit testing just that function.
There was a problem hiding this comment.
nice suggestion @pattisdr, i'd also thought execute_prepared_request was getting rather long. i'll push an update here shortly
There was a problem hiding this comment.
@pattisdr - be7e5e1 has a proposed implementation for the above. i also decided to pull out an _unwrap_response_data function while i was there for similar reasons, but please let me know if you don't think that's appropriate.
and i'm still getting a feel for python static/class/instance methods, so please let me know if you don't think that these should be static methods.
… cover the functions
…nit tests over from integraion test file to new test_saas_connector file
pattisdr
added
run unsafe ci checks
adamsachs
deleted the
457-saas-connector-improve-ignore_errors-option-to-give-the-caller-the-raw-response-object
branch
…ponse (ethyca#462) * allow SaasConnector.send to return raw response object when ignoring errors. move response conversion into higher-level handling method * tweak log message for clarity * split out some utility functions within saas connector. unit tests to cover the functions * add some more test coverage for _handle_errored_response. move some unit tests over from integraion test file to new test_saas_connector file Co-authored-by: Adam Sachs <[email protected]>
…ponse (#462) * allow SaasConnector.send to return raw response object when ignoring errors. move response conversion into higher-level handling method * tweak log message for clarity * split out some utility functions within saas connector. unit tests to cover the functions * add some more test coverage for _handle_errored_response. move some unit tests over from integraion test file to new test_saas_connector file Co-authored-by: Adam Sachs <[email protected]>
Purpose
Fix for #457, allows more flexibility in how
SaasConnector.sendcan be usedVerified locally that sentry access request test is still passing, which relied on the original
ignore_errorsfunctionality (#307)Changes
-move the conversion of errored request further up the stack
Checklist
Run Unsafe PR Checkslabel has been applied, and checks have passed, if this PR touches any external servicesTicket
Fixes #457