refactor rate limit checking code in process_request, ref gesistsa#69

* Testable + unit tests * Reduce cyclomatic complexity * Using semantic variable name (`api_response` vs `tmp`)
chainsawriot · Dec 22, 2022 · 50420ed · 50420ed
1 parent 71c5585
commit 50420ed
Show file tree

Hide file tree

Showing 2 changed files with 73 additions and 24 deletions.
diff --git a/R/utils.R b/R/utils.R
@@ -111,48 +111,36 @@ process_request <- function(token = NULL,
                             page_size = 40L,
                             retryonratelimit = TRUE,
                             verbose = TRUE
-){
+) {
   # if since_id is provided we page forward, otherwise we page backwards
-  if(!is.null(params[["since_id"]])){
+  if(!is.null(params[["since_id"]])) {
     pager <- "since_id"
-  } else{
+  } else {
     pager <- "max_id"
   }
   pages <- ceiling(n/page_size)
   output <- vector("list")
   for(i in seq_len(pages)){
-    tmp <- make_get_request(token = token,path = path,
+    api_response <- make_get_request(token = token,path = path,
                             instance = instance, params = params,
                             anonymous = anonymous)
-
-    if(rate_limit_remaining(tmp)==0){
-      if(isTRUE(retryonratelimit)){
-        wait_until(attr(tmp,"headers")[["rate_reset"]],verbose)
-      } else{
-        output <- c(output,tmp)
-        attr(output,"headers") <- attr(tmp,"headers")
-        sayif(verbose,"rate limit reached and `retryonratelimit=FALSE`. returning current results.")
-        break
-      }
-    }
-    output <- c(output,tmp)
-    attr(output,"headers") <- attr(tmp,"headers")
-    if(is.null(attr(tmp,"headers")[[pager]])){
+    output <- c(output, api_response)
+    attr(output, "headers") <- attr(api_response, "headers")
+    if (break_process_request(api_response = api_response, retryonratelimit = retryonratelimit,
+                              verbose = verbose, pager = pager)) {
       break
     }
-    params[[pager]] <- attr(tmp,"headers")[[pager]]
+    params[[pager]] <- attr(api_response, "headers")[[pager]]
   }
-
   if (isTRUE(parse)) {
-    header <- attr(output,"headers")
+    header <- attr(output, "headers")
 
     output <- FUN(output)
-    attr(output,"headers") <- header
+    attr(output, "headers") <- header
   }
   return(output)
 }
 
-
 ##vectorize function
 v <- function(FUN) {
   v_FUN <- function(x) {
@@ -237,3 +225,19 @@ handle_params <- function(params, max_id, since_id, min_id) {
     }
     params
 }
+
+## a predicate to determine whether to break away from the for-loop of precess_request
+break_process_request <- function(api_response, retryonratelimit = FALSE, verbose = FALSE, pager = "max_id", from = Sys.time()) {
+  if (is.null(attr(api_response,"headers")[[pager]])) {
+    return(TRUE)
+  }
+  if (rate_limit_remaining(api_response) == 0 && isTRUE(retryonratelimit)) {
+    wait_until(until = attr(api_response,"headers")[["rate_reset"]], from = from, verbose = verbose)
+    return(FALSE)
+  }
+  if (rate_limit_remaining(api_response) == 0 && isFALSE(retryonratelimit)) {
+    sayif(verbose,"rate limit reached and `retryonratelimit=FALSE`. returning current results.")
+    return(TRUE)
+  }
+  return(FALSE)
+}
diff --git a/tests/testthat/test-rate_limit.R b/tests/testthat/test-rate_limit.R
@@ -1,4 +1,4 @@
-## wait_until and rate_limit_remaining
+## wait_until ,rate_limit_remaining and break_process_request
 
 test_that("wait_until", {
   x <- capture_message(wait_until(2, 1, verbose = TRUE))
@@ -27,3 +27,48 @@ test_that("rate_limit_remaining", {
   z <- data.frame(x = c(1,2,3,4))
   expect_error(rate_limit_remaining(z))
 })
+
+test_that("break_process_request", {
+  ## rate_limit = 299
+  x1 <- readRDS("../testdata/fake_headers.RDS")
+  expect_false(break_process_request(x1))
+  expect_false(break_process_request(x1, retryonratelimit = TRUE))
+  expect_false(break_process_request(x1, retryonratelimit = TRUE, verbose = TRUE))
+  expect_false(break_process_request(x1, retryonratelimit = FALSE, verbose = TRUE))
+  ## alternative pager
+  x2 <- x1
+  names(attr(x2, "headers"))[4] <- "since_id"
+  expect_false(break_process_request(x2, pager = "since_id"))
+  expect_false(break_process_request(x2, retryonratelimit = TRUE, pager = "since_id"))
+  expect_false(break_process_request(x2, retryonratelimit = TRUE, verbose = TRUE, pager = "since_id"))
+  expect_false(break_process_request(x2, retryonratelimit = FALSE, verbose = TRUE, pager = "since_id"))
+  x3 <- x1
+  attr(x3, "headers")[["max_id"]] <- NULL
+  expect_true(break_process_request(x3, pager = "max_id"))
+  expect_true(break_process_request(x3, retryonratelimit = TRUE, pager = "max_id"))
+  expect_true(break_process_request(x3, retryonratelimit = TRUE, verbose = TRUE, pager = "max_id"))
+  expect_true(break_process_request(x3, retryonratelimit = FALSE, verbose = TRUE, pager = "max_id"))
+  ## emulate `rate_remaining` reaching zero
+  zero_x <- x1
+  attr(zero_x, "headers")[["rate_remaining"]] <- "0"
+  expect_equal(rate_limit_remaining(zero_x), 0)
+  ## retryonratelimit = FALSE
+  expect_true(break_process_request(zero_x, retryonratelimit = FALSE, pager = "max_id"))
+  expect_silent(break_process_request(zero_x, retryonratelimit = FALSE, pager = "max_id"))
+  expect_message(break_process_request(zero_x, retryonratelimit = FALSE, verbose = TRUE, pager = "max_id"), "rate limit reached")
+  ## retryonratelimit = TRUE
+  fake_current_time <- attr(zero_x, "headers")[["rate_reset"]] - 1
+  expect_false(break_process_request(zero_x, retryonratelimit = TRUE, pager = "max_id", from = fake_current_time))
+  expect_silent(break_process_request(zero_x, retryonratelimit = TRUE, pager = "max_id", from = fake_current_time))
+  expect_message(break_process_request(zero_x, retryonratelimit = TRUE, verbose = TRUE, pager = "max_id", from = fake_current_time), "1 second")
+  ## pager is null; retryonratelimit has no effect
+  zero_x_null <- zero_x
+  expect_equal(rate_limit_remaining(zero_x_null), 0)
+  attr(zero_x_null, "headers")[["max_id"]] <- NULL
+  expect_true(break_process_request(zero_x_null, pager = "max_id"))
+  expect_true(break_process_request(zero_x_null, retryonratelimit = TRUE, pager = "max_id"))
+  expect_true(break_process_request(zero_x_null, retryonratelimit = TRUE, verbose = TRUE, pager = "max_id"))
+  expect_true(break_process_request(zero_x_null, retryonratelimit = FALSE, verbose = TRUE, pager = "max_id"))
+  expect_silent(break_process_request(zero_x_null, retryonratelimit = FALSE, verbose = TRUE, pager = "max_id"))
+  expect_silent(break_process_request(zero_x_null, retryonratelimit = TRUE, verbose = TRUE, pager = "max_id"))
+})