ropensci / vcr

Record HTTP calls and replay them
https://docs.ropensci.org/vcr
Other
77 stars 12 forks source link

Error thrown for cassettes that have an empty body #249

Closed KevCaz closed 1 year ago

KevCaz commented 1 year ago

Hi @sckott,

It looks like there is something wrong with cassettes that have a status code that is not 200. I only tried 401 and 404 so not sure about the other codes. (EDIT: @dpprdan pointed out that this is true for cassettes that have an empty body) Here is a reprex:

usethis::create_package("testpkg")
usethis::use_package("httr")
vcr::use_vcr()
file.remove("tests/testthat/test-vcr_example.R")

writeLines("
test_that('default', {
  vcr::use_cassette('get_401', {
    res <- httr::GET('https://httpbin.org/status/401')
  })  
  expect_true(res$status_code == 401)
})

", con = "tests/testthat/test.R")

The first time devtools::test() is used, it works fine, the cassette is recorded correctly. But the second time I get this:

R> devtools::test()
ℹ Testing testpkg
✔ | F W S  OK | Context
✖ | 1       0 | test                                                                                                                                                  
─────────────────────────────────────────────────────────────────
Error (test.R:3): default
Error in `vapply(interactions, function(x) {
    sprintf("%s => %s", request_summary(Request$new()$from_hash(x$request)), 
        response_summary(VcrResponse$new()$from_hash(x$response)))
}, "")`: values must be length 1,
 but FUN(X[[1]]) result is length 0
Backtrace:
 1. vcr::use_cassette(...)
      at test.R:3:2
 2. vcr::insert_cassette(...)
 3. Cassette$new(...)
 4. vcr (local) initialize(...)
 5. self$http_interactions()
 6. HTTPInteractionList$new(...)
 7. vcr (local) initialize(...)
 8. base::vapply(...)

I did not investigate further because I have limited time to work on this right now, I thought I could post this now in case you have an idea how to solve it quickly but if you cannot work on this now I could also investigate latter (assuming there is actually something wrong).

Session Info ```r sessionInfo() R version 4.2.2 (2022-10-31) Platform: x86_64-pc-linux-gnu (64-bit) Running under: Ubuntu 22.04.1 LTS Matrix products: default BLAS: /usr/lib/x86_64-linux-gnu/openblas-pthread/libblas.so.3 LAPACK: /usr/lib/x86_64-linux-gnu/openblas-pthread/libopenblasp-r0.3.20.so locale: [1] LC_CTYPE=en_CA.UTF-8 LC_NUMERIC=C LC_TIME=en_CA.UTF-8 LC_COLLATE=en_CA.UTF-8 LC_MONETARY=en_CA.UTF-8 LC_MESSAGES=en_CA.UTF-8 [7] LC_PAPER=en_CA.UTF-8 LC_NAME=C LC_ADDRESS=C LC_TELEPHONE=C LC_MEASUREMENT=en_CA.UTF-8 LC_IDENTIFICATION=C attached base packages: [1] stats graphics grDevices utils datasets methods base other attached packages: [1] testpkg_0.0.0.9000 vcr_1.1.0 testthat_3.1.5 loaded via a namespace (and not attached): [1] Rcpp_1.0.9 prettyunits_1.1.1 ps_1.7.2 rprojroot_2.0.3 digest_0.6.29 utf8_1.2.2 mime_0.12 R6_2.5.1 backports_1.4.1 [10] httr_1.4.4 pillar_1.8.1 rlang_1.0.6 curl_4.3.3 rstudioapi_0.14 data.table_1.14.4 miniUI_0.1.1.1 urlchecker_1.0.1 whisker_0.4 [19] callr_3.7.3 desc_1.4.2 urltools_1.7.3 devtools_2.4.5 stringr_1.4.1 htmlwidgets_1.5.4 igraph_1.3.5 triebeard_0.3.0 shiny_1.7.3 [28] compiler_4.2.2 httpuv_1.6.6 xfun_0.33 pkgconfig_2.0.3 base64enc_0.1-3 pkgbuild_1.3.1 htmltools_0.5.3 tidyselect_1.2.0 tibble_3.1.8 [37] httpcode_0.3.0 roxygen2_7.2.1 codetools_0.2-18 fansi_1.0.3 crayon_1.5.2 withr_2.5.0 later_1.3.0 waldo_0.4.0 brio_1.1.3 [46] crul_1.3 jsonlite_1.8.3 xtable_1.8-4 lifecycle_1.0.3 magrittr_2.0.3 cli_3.4.1 stringi_1.7.8 cachem_1.0.6 diffobj_0.3.5 [55] remotes_2.4.2 fauxpas_0.5.0 fs_1.5.2 promises_1.2.0.1 xml2_1.3.3 ellipsis_0.3.2 targets_0.14.0 vctrs_0.5.0 rematch2_2.1.2 [64] webmockr_0.8.2 tools_4.2.2 glue_1.6.2 purrr_0.3.5 processx_3.8.0 pkgload_1.3.1 fastmap_1.1.0 yaml_2.3.5 sessioninfo_1.2.2 [73] base64url_1.4 memoise_2.0.1 knitr_1.40 profvis_0.3.7 usethis_2.1.6 ```
dpprdan commented 1 year ago

This is a regression in v1.1.0. The error does not happen in v1.0.2.

AFAICT the error occurs on all requests/responses that have an empty body, not only status_code != 200. (Change all 401 in above example to 200 and it also errors. The same happens if you try to use crul::ok("https://www.google.com") in a cassette.)

The error happens here https://github.com/ropensci/vcr/blob/e4a52cb841e020405a7f175c9205f757d0a32ebe/R/http_interaction_list.R#L121-L125

response_summary() returns an empty string a string of length 0.

I believe the error was introduced by https://github.com/ropensci/vcr/commit/cf5ee5bc88b1bb840e8b7c0d6e1a556151b0454c#diff-a947054c47f80b7252dac92d05ce9294c02a7671022a290403913f0bcfa4c197R150-R151

Essentially this line is missing now: https://github.com/ropensci/vcr/blob/e4a52cb841e020405a7f175c9205f757d0a32ebe/R/request_class.R#L156-L157

Unfortuntely I don't have to time to draft a proper PR now, but this should be a simple fix if I am right.

llrs commented 1 year ago

I think those initial lines in http_interation_list are the ones causing the problem but I don't think the fix is to add if (is.null(x)) x <- "" line as this line still exists https://github.com/ropensci/vcr/blob/master/R/request_class.R#L157 .

KevCaz commented 1 year ago

@dpprdan, I'm changing the tittle accordingly.

dpprdan commented 1 year ago

@llrs Sorry, with "missing" I meant that they are not called now. The fix is to change https://github.com/ropensci/vcr/blob/e4a52cb841e020405a7f175c9205f757d0a32ebe/R/response_class.R#L150 back to body_from(hash[["body"]])

I believe the error was introduced by https://github.com/ropensci/vcr/commit/cf5ee5bc88b1bb840e8b7c0d6e1a556151b0454c#diff-a947054c47f80b7252dac92d05ce9294c02a7671022a290403913f0bcfa4c197R150-R151

You may have to scroll up a few lines to see the change I mean.

llrs commented 1 year ago

I created #252 with the fix you mentioned @dpprdan, in my computer it solved all the issues, but as I don't know why it was commented in the first place I think it would be wise to check well the implications of the change.

sckott commented 1 year ago

Thanks for this issue @KevCaz - and @dpprdan for the troubleshooting and @llrs for the PR (I'll have a look at that) - I'll do my best to remember why I made that change.

dpprdan commented 1 year ago

Something something encoding 😉

llrs commented 1 year ago

Thanks to you @sckott for the package! It has prevented many more troubles than caused them!