VCR playback should terminate if it has a response

[oliyh]

VCR playback should terminate if it has a response (whatever it is). This will prevent a real perform-request interceptor from being invoked even if the playback response should have been used.

[lvh]

FYI I don't think the behavior introduced in that commit is correct. I successfully bisected a bug down to it: https://gist.github.com/lvh/9e765f8999cb6d7f5a93a5abbd994588

I'm not 100% sure I identified the bug correctly, but, let's start with observed behavior:

• When returning a VCR response, :body is now a String instead of the expected parsed map (the underlying API returns JSON)
• When not returning a VCR response, the code works fine (meaning in production for example)
• This behavior is consistent with the interceptor chain ending entirely instead of just terminating the :enter stage; meaning, the :leave stage is not started (which is where the parsing code runs)
• That's not how tripod's terminate is supposed to work, so I think this is because remove-stack deletes the interceptors and then terminates
• I think the correct behavior is to terminate but not remove-stack

[lvh]

I'm also not sure I understand the use case for this behavior: isn't the VCR replaying interceptor supposed to replace the real-HTTP-request-making one, so there's no chance of a real HTTP request being made anyway? Is this change intended to do anything if the user is doing that correctly?

FWIW I tried to replace remove-stack with terminate and that did ostensibly fix the issue. I'll create a PR.

[lvh]

I think I figured out why this failure doesn't occur in the test. Unfortunately, I was unable to debug this particularly closely because I couldn't start a REPL; the issue looks identical to #85 even though it is running in a subdirectory:

(cd /Users/lvh/Projects/martian/vcr; lein update-in :dependencies conj '[nrepl,"1.1.1"]' -- update-in :plugins conj '[cider/cider-nrepl,"0.47.1"]' -- update-in '[:repl-options,:nrepl-middleware]' conj '["cider.nrepl/cider-middleware"]' -- with-profile +dev repl :headless)
Error encountered performing task 'repl' with profile(s): 'base,system,user,provided-martian-vcr,dev-martian-vcr'
java.lang.IllegalArgumentException: Bad artifact coordinates com.github.oliyh:martian:jar::version, expected format is <groupId>:<artifactId>[:<extension>[:<classifier>]]:<version>

I was able to run the tests via lein test and this showed a discrepancy between my stored EDN files in my production project and the ones in the test: the responder already has a parsed body, and thats's what gets saved in EDN:

(def dummy-response
  {:status 200
   :headers {"Content-Type" "application/json"}
   :body {:foo "bar"}})

Whereas in my production martian-hato project, the HTTP client returns :body as a String and relies on the :leave stage of the interceptor stack to turn it into not-a-string.

Therefore I think the underlying problem may be a semantic confusion: is the VCR functionality intended to just cover the actual HTTP request part, or is it intended to be at a higher level? I contend it should just do the HTTP bit, and this is an example of why: if I wanted to paper over most of the behavior in martian, I wouldn't need martian-specific VCR functionality.

[oliyh]

I'm also not sure I understand the use case for this behavior: isn't the VCR replaying interceptor supposed to replace the real-HTTP-request-making one, so there's no chance of a real HTTP request being made anyway? Is this change intended to do anything if the user is doing that correctly?

There are two use cases in my mind:

1. Test mode - where you want the replay to return only the responses it has. If it doesn't have a response, return a 404 or error.
2. Mixed mode - maybe you are running interactively or something - where if it doesn't have a response, it will do a real request to go and fetch one (although in the README, the playback replaces the perform-request interceptor)

Given where it sits, at the end of the interceptor chain, it should persist the "raw" response, i.e. a string body. The playback one will rely on the rest of your interceptor chain being in place to coerce that as json or whatever, which means you can use this as a regression test for your interceptor stack as well.

I agree, VCR is just meant to sit at the perform-request, i.e. HTTP level.