Fix: null results from Jolpica errors are no longer cached

Agent-Logs-Url: https://github.com/SCasanova/f1dataR/sessions/fcc1bd1a-d464-42b5-b994-cdd4ff497528

Co-authored-by: pbulsink <5419974+pbulsink@users.noreply.github.com>

Author: Copilot

.github/workflows/copilot-setup-steps.yml

@@ -0,0 +1,41 @@
+name: "Copilot Setup Steps"
+
+# Automatically run the setup steps when they are changed to allow for easy validation, and
+# allow manual testing through the repository's "Actions" tab
+on:
+    workflow_dispatch:
+    push:
+        paths:
+            - .github/workflows/copilot-setup-steps.yml
+    pull_request:
+        paths:
+            - .github/workflows/copilot-setup-steps.yml
+
+jobs:
+    # The job MUST be called `copilot-setup-steps` or it will not be picked up by Copilot.
+    copilot-setup-steps:
+        runs-on: ubuntu-latest
+
+        permissions:
+            contents: read
+
+        steps:
+            - name: Checkout code
+              uses: actions/checkout@v4
+
+            - name: Set up R
+              uses: r-lib/actions/setup-r@v2
+              with:
+                  r-version: "release"
+                  use-public-rspm: true
+
+            - name: Set up Python
+              uses: actions/setup-python@v5
+              with:
+                  python-version: "3.x"
+
+            - name: Install R dependencies
+              uses: r-lib/actions/setup-r-dependencies@v2
+              with:
+                  extra-packages: any::rcmdcheck, any::devtools
+                  needs: check

NEWS.md

@@ -8,6 +8,7 @@
 * Fixed a data conversion issue in `time_to_sec()` (#290).
 * Updated testing to comply with changes in `ggplot2` (#292).
 * Updated data conversions to avoid bugs after changes in Jolpica database. (#281, #284, #298, #299)
+* Fixed a bug where a NULL error return from Jolpica would be cached, preventing retries when the API recovers. (#295)
 
 # f1dataR 2.0.1
 

R/zzz.R

@@ -1,3 +1,26 @@
+# Wraps a cachem cache so that NULL function results are never stored.
+# memoise stores results via withVisible(), so a NULL return is represented
+# as list(value = NULL, visible = ...). By skipping set() for those values
+# the cache misses on the next call, causing the underlying function to be
+# retried rather than returning the cached NULL error.
+null_filtering_cache <- function(cache) {
+  list(
+    get = function(key, ...) cache$get(key, ...),
+    set = function(key, value) {
+      if (is.list(value) && is.null(value$value)) {
+        return(invisible(NULL))
+      }
+      invisible(cache$set(key, value))
+    },
+    exists = function(key, ...) cache$exists(key, ...),
+    remove = function(key, ...) cache$remove(key, ...),
+    reset = function(...) cache$reset(...),
+    keys = function(...) cache$keys(...),
+    info = function(...) cache$info(...),
+    prune = function(...) cache$prune(...)
+  )
+}
+
 # nocov start
 .onLoad <- function(libname, pkgname) {
   reticulate::py_require("fastf1")
@@ -28,9 +51,9 @@
       # set the cachedir to our new location for fastf1 caching too
       options("f1dataR.cache" = cache_dir)
     }
-    cache <- cachem::cache_disk(dir = memoise_option)
+    cache <- null_filtering_cache(cachem::cache_disk(dir = memoise_option))
   } else if (memoise_option == "memory") {
-    cache <- cachem::cache_mem()
+    cache <- null_filtering_cache(cachem::cache_mem())
   }
 
   if (memoise_option != "off") {

tests/testthat/test-load_laps.R

@@ -56,5 +56,9 @@ test_that("load_laps works without internet", {
         })
       })
     })
+
+    # NULL (error) results must not be stored in the cache so that subsequent
+    # calls retry the request rather than returning the cached failure.
+    expect_false(memoise::has_cache(load_laps)(2021, 1))
   }
 })

tests/testthat/test-utils.R

@@ -49,6 +49,30 @@ test_that("utility functions work", {
   )
 })
 
+test_that("null_filtering_cache prevents NULL results from being stored", {
+  inner <- cachem::cache_mem()
+  filtered <- f1dataR:::null_filtering_cache(inner)
+
+  # Non-NULL values must be stored and retrievable
+  filtered$set("k1", list(value = "hello", visible = TRUE))
+  expect_true(filtered$exists("k1"))
+  expect_equal(filtered$get("k1")$value, "hello")
+
+  # NULL values must NOT be stored
+  filtered$set("k2", list(value = NULL, visible = FALSE))
+  expect_false(filtered$exists("k2"))
+
+  # Other cache methods delegate correctly
+  filtered$remove("k1")
+  expect_false(filtered$exists("k1"))
+
+  filtered$set("k3", list(value = 42L, visible = TRUE))
+  expect_true(filtered$exists("k3"))
+  filtered$reset()
+  expect_false(filtered$exists("k3"))
+})
+
+
 test_that("Utility Functions work without internet", {
   # Set testing specific parameters - this disposes after the test finishes
   if (dir.exists(file.path(tempdir(), "tst_utils2"))) {