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

Case sensitivity of ETag header causes an issue for VCR #434

Closed
myronmarston opened this Issue Jul 17, 2011 · 10 comments

Comments

Projects
None yet
3 participants
@myronmarston
Copy link

myronmarston commented Jul 17, 2011

I've been using fog on a project and wanted to use VCR for stubbing the HTTP stuff. The latest Excon addresses a few issues and I've addressed a few more in the the current master branch of VCR, but there's one more issue I'm trying to sort out.

When VCR records an HTTP interaction, it normalizes the casing of the HTTP headers. Content-Type becomes content-type, as demonstrated here. This causes a problem when using VCR to stub fog for S3 stuff because Fog looks for the ETag header with exact casing here. When VCR plays back the recorded response, it winds up converting 'etag' to 'Etag', and using that in the stubbed response, but that's not the casing fog expects. A NoMethodError results.

Overall, I would consider this a bug in VCR, but fixing it is anything but simple. VCR has the header normalization logic due to the nature of how it was developed: it was initially just a tool for recording Net::HTTP requests with FakeWeb, and it was only later expanded to support other HTTP stubbing libraries and other HTTP client libraries. Net::HTTP normalizes headers to lowercase, and as I added support for new libraries, I preserved this behavior--the idea being that a VCR cassette is an abstraction that should be the same regardless of the HTTP library used.

Long term, I have plans for some changes to the VCR cassette format that would include changing this behavior so that HTTP headers are preserved exactly-as-is--but that's down the road, probably for 2.0. In the short term, I'm OK putting in a slightly hacky work-around to treat the ETag header differently so that on playback it converts etag to ETag rather than Etag.

I started working working on this today but have been stumped by the fact that Rack itself normalizes ETag to Etag (in spite of the fact that it is ETag in the RFC). Check out this gist--it demonstrates this rack behavior. I use rack (well, Sinatra, really) in my tests but I can't really even create a proper end-to-end test for this since I can't get the rack server to return ETag. Thinking about this some more--if I put in the short-term fix of converting etag to ETag on playback, that may break some the use of VCR for some other libraries. Consider the case where there is a library that is an API wrapper for an HTTP API served up by rack--the header will be returned as Etag, and if the client logic is case sensitive (as yours is), it will depend upon casing being Etag.

All that is to say...I'm not sure what the right short-term fix is. Would you consider changing Fog's logic to be tolerant of different casing for HTTP headers (or at least for ETag)? i'd definitely consider this a bug in VCR, not in Fog, but given the fact that it's impossible to return an ETag header with some servers (like Rack) making it case-insensitivity would seem to make the Fog logic more robust anyway.

Long term, I hope to get VCR to the point where it never touches the casing of HTTP headers since there's simply no way to ensure the response is played back the same when it does so.

@geemus

This comment has been minimized.

Copy link
Member

geemus commented Jul 18, 2011

Any reason why you prefer this over the built in stuff with Fog.mock!? (I realize this may be a silly question for the creator of VCR, but I like to ask this early in discussions like this to make sure people know about it).

Changing things for this one particular case probably wouldn't be too hard, but being case insensitive is a performance hit that I'm not the most keen on incurring for a case that (in so far as I'm aware) few people will encounter since most will use it live or with Fog.mock! rather than through VCR (no offense, I hope, but that seems to be my feeling for current usership).

I'll have to think about how else we might do something about this, and/or if I want to relax my performance concerns a bit for convenience. It is probably sensible that rack normalizes things, but unfortunate it does it that way. Anyway, leaving this on the back burner for the time being.

@myronmarston

This comment has been minimized.

Copy link

myronmarston commented Jul 19, 2011

Any reason why you prefer this over the built in stuff with Fog.mock!?

I'm using Fog.mock!, actually. I was trying VCR mostly because of seeing this thread. As the author of VCR, I'd like to ensure it works as advertised, and in combination with Fog, that's no happening :(. I wanted to dig in and see if I could fix the issues. Also, if VCR worked flawlessly (as it should), I'd prefer it simply because I understand exactly how it works. For another developer, I imagine Fog.mock! is a better option (especially if they've never used VCR before).

Changing things for this one particular case probably wouldn't be too hard, but being case insensitive is a performance hit that I'm not the most keen on incurring for a case that (in so far as I'm aware) few people will encounter since most will use it live or with Fog.mock! rather than through VCR (no offense, I hope, but that seems to be my feeling for current usership).

No worries--as I said, I consider this to be a bug in VCR, not a bug in Fog. I just wanted to open a dialog so I'm not trying to figure out the right solution on my own.

I'm going to think some more about a short term fix in VCR.

Thanks!

@geemus

This comment has been minimized.

Copy link
Member

geemus commented Sep 2, 2011

Let me know if you need any help, happy to help out on the VCR side or offer my thoughts. Otherwise I think I'm going to go ahead and close this since I don't think I'm probably going to address it on my end at the moment. Thanks!

@geemus geemus closed this Sep 2, 2011

@myronmarston

This comment has been minimized.

Copy link

myronmarston commented Sep 2, 2011

That's fine--I'm actually getting started on VCR 2.0, and I plan to fix this as part of that. Thanks!

@dmitry

This comment has been minimized.

Copy link

dmitry commented Jun 27, 2014

Still doesn't work. Having:

      ETag: 
      - "\"17077909f23800d73af02978a464b826\""

And it gives me the result:

"Etag"=>"\"3e53e40b6077dbd1251818ba3bf7a80a\""
@geemus

This comment has been minimized.

Copy link
Member

geemus commented Jul 1, 2014

Excon now has case insensitive header lookup, which I suspect may help.

@dmitry are you on the latest versions of excon/fog/vcr?

@dmitry

This comment has been minimized.

Copy link

dmitry commented Jul 1, 2014

@geemus yes, I'm on the latest for all of these gems. With hook_into :excon I had issue with the: VCR::Request initialized with an invalid body. Something similar to the vcr/vcr#407. On the webmock I had no issues with it.

PS. Currently I'm using Fog.mock! but I don't like that method, because it does not test fog/paperclip integration... it just stubs the connection layer of the fog... it's ok for some cases, but not enough in my case.

@geemus

This comment has been minimized.

Copy link
Member

geemus commented Jul 3, 2014

@myronmarston any advice on how to best fix this? Thanks!

@myronmarston

This comment has been minimized.

Copy link

myronmarston commented Jul 3, 2014

@myronmarston any advice on how to best fix this? Thanks!

It's hard to say for sure without seeing the example that is causing the error, but in general this happens when a VCR::Request is passed a body object that is not a string. It only knows how to deal with strings. I've seen this error before when users passed the request body as a hash (or something similar). If Fog/excon support passing a request body as a non-string, they need to encode it properly (e.g. as JSON or application/x-www-form-urlencoded or whatever) before it hits VCR. If they don't support passing that, it could make for a better user experience if they were to give users a clear error when they try to do this.

@geemus

This comment has been minimized.

Copy link
Member

geemus commented Jul 7, 2014

@myronmarston ah, thanks. I suspect there was some confusion as to cause here.

@dmitry it sounds as though your issue may be unrelated to the one above (if I'm not mistaken). Hopefully the notes above can help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment