From 874347242b01315cfa59d4119e34d2796d5871ca Mon Sep 17 00:00:00 2001 From: Jared Knowles Date: Wed, 27 May 2026 11:57:31 -0400 Subject: [PATCH] fix(manifest): actionable errors when USCOGDATA_URL is unset or returns non-JSON `cog_gov_search()` (and every other verb) used to fail with a cryptic `jsonlite` lexical error when the package's placeholder default URL was hit and the server returned an HTML welcome page that got cached as `manifest.json`. Three guards added: 1. `.check_url_configured()` aborts with class `uscogdata_url_not_configured` when the resolved URL is empty or still contains the `REPLACE_WITH_SHARE_TOKEN` sentinel. Message names both `Sys.setenv(USCOGDATA_URL = ...)` and `options(uscogdata.url = ...)` remediations and points at the bundled fixture. 2. `.fetch_or_cache_manifest()` parses the response body before persisting it. Non-JSON payloads raise class `uscogdata_invalid_manifest` (URL, Content-Type, parse error) and never touch the on-disk cache. 3. Cache writes are atomic via a sibling tempfile + `file.rename`, and existing caches with non-JSON content are silently refetched instead of returning a parse error to the caller. Local-path manifests that aren't valid JSON now surface the same `uscogdata_invalid_manifest` class with file context. --- NEWS.md | 22 +++++++ R/manifest.R | 110 ++++++++++++++++++++++++++++++--- R/session.R | 1 + tests/testthat/test-manifest.R | 101 ++++++++++++++++++++++++++++++ 4 files changed, 225 insertions(+), 9 deletions(-) create mode 100644 tests/testthat/test-manifest.R diff --git a/NEWS.md b/NEWS.md index edff0ba..375f877 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,27 @@ # uscogdata 0.1.0 (development) +## Clearer errors when `USCOGDATA_URL` is unconfigured or returns non-JSON + +* `cog_open()` now aborts with the `uscogdata_url_not_configured` error + class when the resolved corpus URL still contains the placeholder + `REPLACE_WITH_SHARE_TOKEN` sentinel (or is empty). The message lists both + remediation paths (`Sys.setenv(USCOGDATA_URL = ...)` and + `options(uscogdata.url = ...)`) and points at the bundled fixture for + offline testing. Previously the package proceeded to fetch the placeholder + URL, cached the resulting HTML welcome page, and failed downstream with a + cryptic `jsonlite` lexical-error. +* `.fetch_or_cache_manifest()` now parses the HTTP response body before + persisting it. Non-JSON responses (login pages, 404 HTML) raise + `uscogdata_invalid_manifest` with the URL, Content-Type, and underlying + parse error — and never write to the on-disk cache. +* Manifest cache writes are now atomic (write to `manifest.json.tmp.` + in `cache_dir`, then `file.rename` over the target), so an interrupted + fetch cannot replace a previously-good cache. +* Existing caches with non-JSON content (poisoned by the prior code path) + are silently refetched instead of returning a parse error to the caller. +* Local `USCOGDATA_URL` paths whose `manifest.json` is not valid JSON now + surface the same `uscogdata_invalid_manifest` class with file context. + ## Per-capita denominators now use per-year Census F-33 population * `cog_spending()` and `cog_revenue()` previously divided all years' amounts diff --git a/R/manifest.R b/R/manifest.R index bd2caf1..bc7eb70 100644 --- a/R/manifest.R +++ b/R/manifest.R @@ -1,5 +1,66 @@ # R/manifest.R +# Sentinel substring baked into the placeholder default URL. If we see this +# in the resolved URL, the user hasn't configured USCOGDATA_URL yet. +.PLACEHOLDER_TOKEN <- "REPLACE_WITH_SHARE_TOKEN" + +#' Abort with actionable guidance when the resolved corpus URL is still the +#' placeholder shipped with the package (or any URL containing the sentinel). +#' Called from `cog_open()` before any I/O so users see a clear message +#' instead of a downstream JSON parse error. +#' @noRd +.check_url_configured <- function(url) { + if (!is.character(url) || length(url) != 1L || !nzchar(url)) { + cli::cli_abort(c( + "USCOGDATA_URL is not configured.", + i = "Set the corpus location via one of:", + "*" = "{.code Sys.setenv(USCOGDATA_URL = \"/\")}", + "*" = "{.code options(uscogdata.url = \"/\")}", + i = "For an offline smoke test, use the bundled fixture: {.code system.file(\"extdata/fixture_corpus\", package = \"uscogdata\")}." + ), class = "uscogdata_url_not_configured") + } + if (grepl(.PLACEHOLDER_TOKEN, url, fixed = TRUE)) { + sentinel <- .PLACEHOLDER_TOKEN + cli::cli_abort(c( + "USCOGDATA_URL is not configured (placeholder URL detected).", + x = "Current value contains the sentinel {.val {sentinel}}: {.url {url}}", + i = "Set the corpus location via one of:", + "*" = "{.code Sys.setenv(USCOGDATA_URL = \"/\")}", + "*" = "{.code options(uscogdata.url = \"/\")}", + i = "For an offline smoke test, use the bundled fixture: {.code system.file(\"extdata/fixture_corpus\", package = \"uscogdata\")}.", + i = "For the live Civilytics corpus, request the Nextcloud share URL from the package maintainer." + ), class = "uscogdata_url_not_configured") + } + invisible(url) +} + +#' Try to parse a JSON file. Returns parsed object on success, NULL on +#' any parse failure (so callers can decide whether to refetch). +#' @noRd +.try_parse_manifest_file <- function(path) { + tryCatch( + jsonlite::fromJSON(path, simplifyVector = FALSE), + error = function(e) NULL + ) +} + +#' Abort with a clear, classified error when a manifest payload (string or +#' file) cannot be parsed as JSON. Surfaces the URL, content-type if known, +#' and the underlying parse error. +#' @noRd +.abort_invalid_manifest <- function(source, content_type = NA_character_, parse_error = NULL) { + ct <- if (is.na(content_type) || !nzchar(content_type)) "" else content_type + pmsg <- if (is.null(parse_error)) "" else conditionMessage(parse_error) + cli::cli_abort(c( + "Corpus manifest is not valid JSON.", + x = "Source: {source}", + i = "Content-Type: {ct}", + i = "Likely causes: USCOGDATA_URL points at a login page, a 404 HTML page, or the wrong share; or the corpus has not been published yet.", + i = "Set USCOGDATA_URL to a directory (local path or HTTPS) that serves manifest.json directly.", + if (nzchar(pmsg)) c(">" = "Parse error: {pmsg}") else NULL + ), class = "uscogdata_invalid_manifest") +} + #' Fetch manifest.json from URL (or read from a local fixture path), #' cache locally, validate TTL. #' @noRd @@ -11,23 +72,54 @@ if (!file.exists(local_manifest)) { cli::cli_abort("Local fixture has no manifest.json at {local_manifest}") } - return(jsonlite::fromJSON(local_manifest, simplifyVector = FALSE)) + return(tryCatch( + jsonlite::fromJSON(local_manifest, simplifyVector = FALSE), + error = function(e) .abort_invalid_manifest(source = local_manifest, parse_error = e) + )) } cache_path <- file.path(cache_dir, "manifest.json") ttl <- as.integer(.cfg("manifest_ttl_secs")) - needs_fetch <- !file.exists(cache_path) || - difftime(Sys.time(), file.info(cache_path)$mtime, units = "secs") > ttl + cache_fresh <- file.exists(cache_path) && + difftime(Sys.time(), file.info(cache_path)$mtime, units = "secs") <= ttl - if (needs_fetch) { - resp <- httr2::request(paste0(url, "manifest.json")) |> - httr2::req_error(is_error = function(r) httr2::resp_status(r) >= 400) |> - httr2::req_perform() - writeLines(httr2::resp_body_string(resp), cache_path) + # Honor a fresh cache only if its contents still parse as JSON. A previous + # version of this package could write HTML directly into the cache; treat + # such poisoned caches as if they were missing so the next call recovers. + if (cache_fresh) { + parsed <- .try_parse_manifest_file(cache_path) + if (!is.null(parsed)) return(parsed) } - jsonlite::fromJSON(cache_path, simplifyVector = FALSE) + resp <- httr2::request(paste0(url, "manifest.json")) |> + httr2::req_error(is_error = function(r) httr2::resp_status(r) >= 400) |> + httr2::req_perform() + body <- httr2::resp_body_string(resp) + + # Parse BEFORE persisting. If the server returned HTML / a login page / + # any non-JSON body with a 2xx status, we must not write it to the cache. + parsed <- tryCatch( + jsonlite::fromJSON(body, simplifyVector = FALSE), + error = function(e) { + ct <- tryCatch(httr2::resp_content_type(resp), error = function(e2) NA_character_) + .abort_invalid_manifest( + source = paste0(url, "manifest.json"), + content_type = ct, + parse_error = e + ) + } + ) + + # Atomic write: tmp file alongside cache_path (same filesystem -> no EXDEV) + # then rename. Ensures a partial write or interrupted process never + # replaces a previously-good cache. + if (!dir.exists(cache_dir)) dir.create(cache_dir, recursive = TRUE) + tmp <- paste0(cache_path, ".tmp.", Sys.getpid()) + on.exit(if (file.exists(tmp)) unlink(tmp), add = TRUE) + writeLines(body, tmp) + file.rename(tmp, cache_path) + parsed } #' @noRd diff --git a/R/session.R b/R/session.R index fc83819..422469e 100644 --- a/R/session.R +++ b/R/session.R @@ -5,6 +5,7 @@ #' @noRd cog_open <- function(url = .resolve_url(), cache_dir = .resolve_cache_dir()) { + .check_url_configured(url) if (!dir.exists(cache_dir)) dir.create(cache_dir, recursive = TRUE) con <- DBI::dbConnect(duckdb::duckdb()) diff --git a/tests/testthat/test-manifest.R b/tests/testthat/test-manifest.R new file mode 100644 index 0000000..0c4a20c --- /dev/null +++ b/tests/testthat/test-manifest.R @@ -0,0 +1,101 @@ +# tests/testthat/test-manifest.R +# +# Tests for the guards on .fetch_or_cache_manifest() and cog_open() that +# protect users from silent failures when USCOGDATA_URL is misconfigured +# or returns non-JSON content. + +test_that("cog_open aborts with actionable error when URL is the placeholder default", { + uscogdata:::cog_close() + on.exit(uscogdata:::cog_close(), add = TRUE) + + placeholder <- "https://cloud.civilytics.org/s/REPLACE_WITH_SHARE_TOKEN/download/" + withr::with_envvar(c(USCOGDATA_URL = placeholder), { + expect_error( + uscogdata:::cog_open(), + class = "uscogdata_url_not_configured" + ) + }) +}) + +test_that("placeholder guard fires for any URL containing the sentinel token", { + uscogdata:::cog_close() + on.exit(uscogdata:::cog_close(), add = TRUE) + + # Sentinel detection should be substring-based — covers any host that still + # has REPLACE_WITH_SHARE_TOKEN baked in (default or partial user edit). + withr::with_envvar(c(USCOGDATA_URL = "https://other.example/s/REPLACE_WITH_SHARE_TOKEN/x/"), { + expect_error( + uscogdata:::cog_open(), + class = "uscogdata_url_not_configured" + ) + }) +}) + +test_that("placeholder guard error names both env var and option as remediation", { + uscogdata:::cog_close() + on.exit(uscogdata:::cog_close(), add = TRUE) + + placeholder <- "https://cloud.civilytics.org/s/REPLACE_WITH_SHARE_TOKEN/download/" + withr::with_envvar(c(USCOGDATA_URL = placeholder), { + msg <- tryCatch(uscogdata:::cog_open(), error = conditionMessage) + expect_match(msg, "USCOGDATA_URL", fixed = TRUE) + expect_match(msg, "uscogdata.url", fixed = TRUE) + }) +}) + +test_that("local manifest containing HTML produces uscogdata_invalid_manifest, not raw parse error", { + uscogdata:::cog_close() + on.exit(uscogdata:::cog_close(), add = TRUE) + + tmp <- withr::local_tempdir() + writeLines( + c("", " Welcome to our server", ""), + file.path(tmp, "manifest.json") + ) + + withr::with_envvar(c(USCOGDATA_URL = paste0(tmp, "/")), { + err <- expect_error( + uscogdata:::cog_open(), + class = "uscogdata_invalid_manifest" + ) + expect_match(conditionMessage(err), "manifest", ignore.case = TRUE) + }) +}) + +test_that("remote manifest fetch does not poison cache when response is HTML", { + uscogdata:::cog_close() + on.exit(uscogdata:::cog_close(), add = TRUE) + + tmp_cache <- withr::local_tempdir() + cache_path <- file.path(tmp_cache, "manifest.json") + + # Pretend the cache already exists with stale-but-fresh-by-mtime HTML + # (simulating a previous poisoned write from the old behavior). When the + # fetcher sees invalid JSON in the cache, it must refetch rather than + # silently returning a parse error to the caller. + writeLines("poisoned", cache_path) + Sys.setFileTime(cache_path, Sys.time()) # ensure within TTL + + # We don't have a live HTTP fixture here, so the refetch will fail at the + # network layer — but the failure should NOT be a jsonlite parse error on + # the cached HTML; it should be a network-level httr2 error. The cache + # file itself must remain untouched (no atomic-write half-states). + withr::with_envvar( + c( + USCOGDATA_URL = "https://invalid.localhost.uscogdata.test/", + USCOGDATA_CACHE_DIR = tmp_cache + ), + { + err <- tryCatch(uscogdata:::cog_open(), error = identity) + expect_s3_class(err, "error") + # Must not be a JSON lexical error on HTML. + expect_false(grepl("lexical error", conditionMessage(err), fixed = TRUE)) + } + ) + + # Atomic write contract: no stray tmp files left behind in cache_dir. + expect_length( + list.files(tmp_cache, pattern = "manifest\\.json\\.tmp"), + 0L + ) +})