Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dont attempt to parse a invalid response(nil) when using hook_into :excon #916

Merged

Conversation

andrehjr
Copy link
Contributor

Hi 馃檶

When using hook_into :excon and dealing with an internal error from Excon(i.e: OpenSSL::SSL::SSLError) VCR::Middleware::Excon::Response#error_call was breaking because it was trying to parse/record a nil response. The error happened here on Logger#response_summary, because response is nil.

I think that the correct behavior would be to let Excon throw its own error up the stack as there's nothing recorded yet here and there's no need for VCR to attempt to read an empty response for this scenario. Please, correct me if I'm wrong.

When using hook_into :webmock the error is thrown and no cassette is recorded. Same things happens here now.

Fixes #651

olleolleolle
olleolleolle previously approved these changes Jan 1, 2022
Copy link
Member

@olleolleolle olleolleolle left a comment

Choose a reason for hiding this comment

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

I think this looks like a great addition!

Thanks for the good context given in the Description.

Can you add a CHANGELOG line about the change?

spec/lib/vcr/library_hooks/excon_spec.rb Outdated Show resolved Hide resolved
@andrehjr andrehjr force-pushed the fix-excon-middleware-errors-before-body branch from 88b77db to f7a8a69 Compare January 2, 2022 00:28
@andrehjr
Copy link
Contributor Author

andrehjr commented Jan 2, 2022

Done @olleolleolle ! Thanks for the review 馃檶

Copy link
Member

@olleolleolle olleolleolle left a comment

Choose a reason for hiding this comment

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

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

VCR Does Not Support Recording Broken SSL Certs
2 participants