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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Parse headers from aggregated proxy requests/responses #681

Conversation

ajroetker
Copy link
Contributor

Prior to this commit when connecting to a proxy with auth, faraday
adapters would cause faraday to fail by first trying to connect without
auth and then connection again with auth, faraday could not handle the
response headers from the aggregate response of the two requests.
This commit addresses the above issue by changing faradays parse
function for headers to handle the case where there are multiple
repsonse headers by attempting to parse the last set of response
headers.

Prior to this commit when connecting to a proxy with auth, faraday
adapters would cause faraday to fail by first trying to connect without
auth and then connection again with auth, faraday could not handle the
response headers from the aggregate response of the two requests.
This commit addresses the above issue by changing faradays parse
function for headers to handle the case where there are multiple
repsonse headers by attempting to parse the last set of response
headers.
@ajroetker
Copy link
Contributor Author

Not sure what's up with 1.9.3 tests, there don't appear to be any related warnings when I run the test script invocation locally.

@iMacTia
Copy link
Member

iMacTia commented Apr 11, 2017

Hi @ajroetker and thanks for the PR.
I can see there are issue with ruby 1.9.3 related to the new slice_before method.
Unfortunately I'm not really sure of where the issue lies as the warning seems related to something outside Faraday.
I'll need more time to investigate, or maybe you can get the same result without using slice_before?

@iMacTia iMacTia added bug and removed feature labels Apr 11, 2017
@iMacTia
Copy link
Member

iMacTia commented Apr 11, 2017

Also, could you provide a test case to demonstrate the issue you described above?
Thanks

@ajroetker
Copy link
Contributor Author

@iMacTia Thanks for the review, I added a test case and removed the usage of slice_before

Copy link
Member

@iMacTia iMacTia left a comment

Choose a reason for hiding this comment

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

Hi @ajroetker and sorry for the slow reply.
Please just make the method more efficient and I'll be happy to merge this in

# Find the last set of response headers.
last_response = headers.reverse.take(
headers.reverse.find_index { |x| x.match(/^HTTP\//) } + 1
).reverse
Copy link
Member

Choose a reason for hiding this comment

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

The above is extremely inefficient.
Please change it to:

start_index = headers.rindex { |x| x.match(/^HTTP\//) }
last_response = headers.slice(start_index, headers.size)

Or happy to review an even better proposal 😄

@ajroetker ajroetker force-pushed the parse_headers_from_aggregated_proxy_requests branch from 817e6ea to a8d755e Compare April 24, 2017 18:11
@ajroetker
Copy link
Contributor Author

@iMacTia Thanks for the review, I pushed up your suggested changes!

This commit adds a test for the aggregated header parsing code.
This commit also avoid the use of slice_before which was causing issues
with the linter in travis ci.
@ajroetker ajroetker force-pushed the parse_headers_from_aggregated_proxy_requests branch from a8d755e to ac31d37 Compare April 24, 2017 20:03
@iMacTia
Copy link
Member

iMacTia commented Apr 25, 2017

Thanks @ajroetker.
This will probably go in v0.12.2

@iMacTia iMacTia merged commit b2e8bcc into lostisland:master Apr 25, 2017
headers = header_string.split(/\r\n/)

# Find the last set of response headers.
start_index = headers.rindex { |x| x.match(/^HTTP\//) }

Choose a reason for hiding this comment

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

I'm getting an issue where this returns nil when replaying a cassette from WebMock, because there's no HTTP ... line in the replayed headers. Is this a bug in Faraday or in WebMock?

Copy link
Member

Choose a reason for hiding this comment

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

That's really strange.
I haven't used Webmock cassettes but server responses should always start with the HTTP version. I suppose Webmock authors didn't think that was important and removed it from the cassette. I would suggest to open a ticket with them and double-check

Choose a reason for hiding this comment

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

Just for reference, it was a bug in VCR. Someone has already fixed it in a PR though 😄 vcr/vcr#657

Copy link

Choose a reason for hiding this comment

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

@iMacTia Hey, I ran into this as well. Agree this is a VCR bug but since the latest release (3.0.3) was over a year ago I don't expect a quick fix.
Would it be possible to add a .to_i cast here to keep it compatible?
For now the easiest solution is to downgrade faraday to 0.12.1

Copy link
Member

Choose a reason for hiding this comment

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

@bartoszkopinski can you please explain better where you would add the .to_i?
But please keep in mind that I'm not willing to add ugly patches to Faraday only to make it compatible with an outdated gem.
The solution in that case would be to either fix the gem (you can always fork it if it's dead), or monkey-patch tin your project

Copy link

Choose a reason for hiding this comment

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

@iMacTia I think headers.rindex { |x| x.match(/^HTTP\//) }.to_i should fix the problem (but I didn't test it).
If I understand correctly, this would simply cause it to start from the top, if HTTP header is missing. It's not VCR specific as I believe faraday shouldn't fail in this place anyway (the change is basically backwards incompatible by assuming the server response will always contain a certain string).
Looking at VCR code I now believe this was a conscious decision to remove it so it's even less likely to be fixed.

Copy link
Member

Choose a reason for hiding this comment

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

I see, I understand what you mean now.
I'll have a look at rindex implementation, I think what we're trying to add here is a fallback value in case there's no "HTTP" header. The maybe .to_i is not the best solution for that, but I get your point now!

Copy link
Member

Choose a reason for hiding this comment

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

According to this: https://ruby-doc.org/core-2.3.0/Array.html#method-i-rindex
I believe the best (or clearest, at least) implementation would then be:

start_index = headers.rindex { |x| x.match(/^HTTP\//) } || 0

Copy link
Member

Choose a reason for hiding this comment

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

Fixed in #719

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

Successfully merging this pull request may close these issues.

None yet

4 participants