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

curl support #40

Closed
nbenn opened this issue May 14, 2018 · 20 comments
Closed

curl support #40

nbenn opened this issue May 14, 2018 · 20 comments
Labels

Comments

@nbenn
Copy link
Member

nbenn commented May 14, 2018

I'm interested in using webmockr for a test suite of mine, but I'm currently using jeroen/curl for making http requests.

Are there plans on supporting curl or is this not happening anytime soon?

@sckott
Copy link
Collaborator

sckott commented May 14, 2018

thanks for the nudge @nbenn

Integrating with a http client library (e.g., curl) does require tweaking the curl package a tiny bit to have a hook for webmockr. I'll see if he's interested in doing it.

what functions do you use in curl that you'd want to mock?

@nbenn
Copy link
Member Author

nbenn commented May 16, 2018

Thanks for the quick response.

The package I'm hoping to use webmockr with uses both easy handles (curl::curl_fetch_memory()) as well as the multi interface (curl::curl_fetch_multi()/curl::multi_run()).

@sckott
Copy link
Collaborator

sckott commented May 16, 2018

thanks for the clarification @nbenn - will update on any progress

@sckott sckott added the adapter label May 16, 2018
@nbenn
Copy link
Member Author

nbenn commented May 18, 2018

Let me know if there is anything I can do to help. I'm happy to.

@sckott
Copy link
Collaborator

sckott commented May 18, 2018

@nbenn okay, try after installing like remotes::install_github("ropensci/webmockr@adapter-curl"), which will also install a fork of curl pkg from my gh account

see examples here https://github.com/ropensci/webmockr/blob/adapter-curl/R/adapter-curl.R#L37-L66

it's a bit of a hack - it's not easy to inspect the curl handle so we use curl::curl_echo to make a fake request to localhost, which gives us the headers, http method, etc, which we can then pass through webmockr to mock the request, so no real HTTP request is done to the web, but we do need to do a localhost request

thx to @jeroen for help. he and I will talk more about possibly avoiding doing the curl_echo call

@nbenn
Copy link
Member Author

nbenn commented Jun 6, 2018

@sckott thanks a lot for moving so quickly with this and I am sorry that it took a bit of time for me to have a look at it.

Naturally, down the line, I'm interested in getting webmockr and vcr to work with curl.

But first, in order to get the automated recording of requests working, we'd need the webmockr_net_connect_allowed section of the curl adapter to work. This seems doable, as (apart from the hacky curl::mock(FALSE)/curl::mock(TRUE), currently the problem is that req$called does not hold the necessary information to evaluate the original curl call. But this does not seem like a big problem either, as we could just modify the curl functions to return the function calls, e.g. nbenn/curl@50571d8 and evaluate this later as

curl::mock(FALSE) # tempoarily disable
resp <- eval(req$called)
curl_resp <- build_curl_response(req, resp)
curl::mock(TRUE) # re-enable

Or am I missing something here? Are you interested in PRs?

@sckott
Copy link
Collaborator

sckott commented Jun 7, 2018

yeah, PRs welcome. makes total sense you'd need net connect allowed working. does nbenn/curl@50571d8 make net connect allowed work?

does the use of curl_echo bother you at all? or are you okay with that? It sounds like Jeroen may be able to sort out a curl handle inspection thing some day, but it wouldn't happen any time soon.

@sckott
Copy link
Collaborator

sckott commented Jun 13, 2018

fix for allow net connect for curl adapter in 850b31f

@sckott
Copy link
Collaborator

sckott commented Jun 13, 2018

@nbenn can you reinstall remotes::install_github("ropensci/webmockr@adapter-curl") and try real connections again with webmockr loaded

@sckott
Copy link
Collaborator

sckott commented Jul 27, 2018

@nbenn did you get a chance to try this yet?

@nbenn
Copy link
Member Author

nbenn commented Jul 29, 2018

@sckott I haven't tried any webmockr related stuff, sorry. I though I'd wait until the infx review process is over, before taking this up again.

@sckott
Copy link
Collaborator

sckott commented Jul 31, 2018

ok, thanks

@sckott
Copy link
Collaborator

sckott commented Jan 10, 2019

changes in 3b05bc1

@sckott
Copy link
Collaborator

sckott commented Jan 10, 2019

@nbenn if you have a chance, just updated the integration with curl - reinstall from remotes::install_github("ropensci/webmockr@adapter-curl")

@nbenn
Copy link
Member Author

nbenn commented Mar 31, 2019

@sckott sorry for the crazy long delay on my side. I got round to testing vcr/webmockr in conjunction with curl. The short story: with the current state of things I don't get very far. I'm happy to elaborate a bit:

For me, webmockr fails when trying to evaluate the original call, because req$called holds a string rather that a proper call/expression object. My call stack at the point of failure looks something like

...
16: curl::curl_fetch_memory(as.character(eval(urls[[i]])), handle = create_hand...
17: webmockr::curl_mock_req(url, handle, called = called)
18: adap$handle_request(req)
19: eval(parse(text = paste0("curl::", strsplit(req$called, "\\\"")[[1]][1])))
20: parse(text = paste0("curl::", strsplit(req$called, "\\\"")[[1]][1]))

and req$called holds

"curl::curl_fetch_memoryas.characterevalurls[[i]], handle = create_handlebodies[[i]]"

finally, the error that is thrown is

Error in parse(text = paste0("curl::", strsplit(req$called, "\\\"")[[1]][1])) :
  <text>:1:11: unexpected '::'
1: curl::curl::

Of course, only selectively pasting "curl::" in front of req$called will not fix the real problem, as this call can no longer be evaluated.

What reason do you have for doing

called <- gsub("\\(|\\)", "", deparse(sys.call()))

You already have url and handle, which suffice to re-build the original call later on, no? Are you only after the function name? In that case, why not something like deparse(match.call()[[1L]])?

@nbenn
Copy link
Member Author

nbenn commented Mar 31, 2019

Is vcr supposed to work in conjunction with the curl branch of webmockr? I can't get it to store a response.

@sckott
Copy link
Collaborator

sckott commented Apr 1, 2019

Is vcr supposed to work in conjunction with the curl branch of webmockr? I can't get it to store a response.

I thought I had started work on curl integration in vcr, but I don't think I have. I'll start a branch for that. we need a https://github.com/ropensci/vcr/blob/master/R/request_handler-crul.R equivalent for curl

@sckott
Copy link
Collaborator

sckott commented Apr 1, 2019

thanks for testing this @nbenn

I'm not surprised you ran into trouble as I probably only tested it in a very narrow set of cases.

First, note that I'm not positive @jeroen will integrate webmockr in curl - see the PR at jeroen/curl#174 (comment) - in a call somewhat recently we talked about a different approach than what is being used now, i'm not sure what jeroen's current thinking is on webmocrk in curl

for

called <- gsub("\(|\)", "", deparse(sys.call()))

i don't know for sure why, i think your pr is better

@sckott
Copy link
Collaborator

sckott commented Apr 1, 2019

your two PR's are merged now.

@sckott
Copy link
Collaborator

sckott commented Nov 7, 2019

@nbenn sorry for long delay on this, jeroen and I talked again the other day and we decided curl wont integrate with webmockr mostly b/c curl is just too important to possibly introduce edge case bugs into

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

No branches or pull requests

2 participants