Error thrown for cassettes that have an empty body

[KevCaz]

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

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]

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

vcr/R/http_interaction_list.R

Lines 121 to 125 in e4a52cb

	interaction_summaries <- vapply(interactions, function(x) {
	sprintf("%s => %s",
	request_summary(Request$new()$from_hash(x$request)),
	response_summary(VcrResponse$new()$from_hash(x$response)))
	}, "")

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

I believe the error was introduced by cf5ee5b#diff-a947054c47f80b7252dac92d05ce9294c02a7671022a290403913f0bcfa4c197R150-R151

Essentially this line is missing now:

vcr/R/request_class.R

Lines 156 to 157 in e4a52cb

	body_from <- function(x) {
	if (is.null(x)) x <- ""

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]

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]

@dpprdan, I'm changing the tittle accordingly.

[dpprdan]

@llrs Sorry, with "missing" I meant that they are not called now. The fix is to change

vcr/R/response_class.R

Line 150 in e4a52cb

hash[["body"]],

back to
body_from(hash[["body"]])

I believe the error was introduced by cf5ee5b#diff-a947054c47f80b7252dac92d05ce9294c02a7671022a290403913f0bcfa4c197R150-R151

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

[llrs]

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]

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]

Something something encoding 😉

[llrs]

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