Closed oliyh closed 11 months ago
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:
:body
is now a String instead of the expected parsed map (the underlying API returns JSON)terminate
is supposed to work, so I think this is because remove-stack
deletes the interceptors and then terminatesterminate
but not remove-stack
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.
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 https://github.com/oliyh/martian/issues/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.
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:
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.
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.