From 748ca4a56e3bcab24f54adbc5aef0f2c63fa91a0 Mon Sep 17 00:00:00 2001 From: Jared Knowles Date: Sat, 25 Jul 2026 18:37:42 -0400 Subject: [PATCH] fix(#3): normalize the corpus URL's trailing slash at resolution Investigating "gov search doesn't work" (#3) turned up two separate things. THE REPORTED SYMPTOM IS ALREADY FIXED. #3 reported `cog_gov_search("Orange")` dying in jsonlite with `lexical error: invalid char in json text. `. That was fixed the same day the issue was filed, by 8743472 "fix(manifest): actionable errors when USCOGDATA_URL is unset or returns non-JSON" (issue filed 2026-05-27 11:30; commit 2026-05-27). The issue was simply never closed. Verified now: injecting an HTML manifest.json raises a typed `uscogdata_invalid_manifest` condition naming the likely causes, with the raw parse error demoted to a footnote, and `cog_gov_search("Orange")` returns 62 rows against the live corpus. THE ROOT CAUSE OF THAT HTML WAS STILL LIVE -- and is what this commit fixes. Every consumer builds locations by CONCATENATION: manifest.R:95 paste0(url, "manifest.json") mirror.R:48,125 paste0(url, e$path) views.R the parquet glob and mirror.R:104 documents the invariant outright ('url ends in "/"'). The error messages tell users to set `"/"`. But `.resolve_url()` was a bare `.cfg("url")` passthrough -- the invariant was assumed everywhere and enforced nowhere. So a URL entered without the slash failed silently and misleadingly: HTTPS -> ".../downloadmanifest.json"; the host answers with an HTML 404 page, which lands in the JSON parser as EXACTLY the #3 symptom -- and the guard then blames "login page / 404 / wrong share" when the real cause was one missing character. local -> ".../corpusdata/long/**/*.parquet" and a DuckDB "No files found". Reproduced both: pointing USCOGDATA_URL at the bundled fixture without a trailing slash gave No files found that match ".../fixture_corpusdata/long/**/*.parquet" Normalizing once at resolution fixes every consumer at the same time, rather than having each call site re-derive the invariant. An empty setting passes through untouched so manifest.R's "not configured" guard still fires instead of the value degrading into a bare "/" filesystem root. RED->GREEN: 4 tests added, 2 failed first (append-missing-slash, local-path normalization); the already-correct cases (slash present, empty setting) passed throughout and pin them against regression. Same fixture path that produced the DuckDB error above now returns 62 rows. Suite: FAIL 0 | WARN 0 | SKIP 0 | PASS 471 (was 463; +8 = the new tests). --- R/config.R | 25 +++++++++++++++++++++++- tests/testthat/test-config.R | 38 ++++++++++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+), 1 deletion(-) diff --git a/R/config.R b/R/config.R index 6ebc78f..a5534b1 100644 --- a/R/config.R +++ b/R/config.R @@ -21,7 +21,30 @@ .uscogdata_defaults[[key]] } -.resolve_url <- function() .cfg("url") +#' Resolve the corpus URL, guaranteeing the trailing slash the package assumes. +#' +#' Every consumer builds locations by CONCATENATION -- `paste0(url, +#' "manifest.json")` in manifest.R, `paste0(url, e$path)` in mirror.R, and the +#' parquet glob in views.R -- and mirror.R:104 documents the invariant outright +#' ('url ends in "/"'). Nothing enforced it, so a URL entered without the slash +#' failed silently and misleadingly: +#' +#' HTTPS -> ".../downloadmanifest.json"; the host answers with an HTML 404 +#' page, which lands in the JSON parser as the lexical error +#' reported in issue #3 -- pointing the user at "login page / wrong +#' share" when the real cause was one missing character. +#' local -> ".../corpusdata/long/**/*.parquet" and a DuckDB "No files found". +#' +#' Normalizing here fixes every consumer at once, rather than each call site +#' re-deriving the same invariant. An empty setting is passed through +#' untouched so manifest.R's "not configured" guard still fires instead of the +#' value degrading into a bare "/" filesystem root. +#' @noRd +.resolve_url <- function() { + url <- .cfg("url") + if (is.null(url) || !nzchar(url) || grepl("/$", url)) return(url) + paste0(url, "/") +} .resolve_cache_dir <- function() { v <- .cfg("cache_dir") diff --git a/tests/testthat/test-config.R b/tests/testthat/test-config.R index 8356046..3461ab2 100644 --- a/tests/testthat/test-config.R +++ b/tests/testthat/test-config.R @@ -26,3 +26,41 @@ test_that(".resolve_cache_dir falls back to R_user_dir", { }) }) }) + +# --------------------------------------------------------------------------- +# Trailing-slash normalization (uscogdata #3 follow-up). +# +# EVERY consumer builds paths by concatenation: paste0(url, "manifest.json") +# (manifest.R), paste0(url, e$path) (mirror.R), and the parquet glob in +# views.R. mirror.R:104 even comments 'url ends in "/"' -- an assumption the +# package documents and relies on but never enforced. +# +# A URL missing its trailing slash therefore fails SILENTLY and confusingly: +# HTTPS -> ".../downloadmanifest.json" -> the host answers with an HTML 404 +# page -> the jsonlite lexical error that issue #3 reported; +# local -> ".../corpusdata/long/**/*.parquet" -> DuckDB "No files found". +# Neither message points at the real cause. Normalize once, at resolution. +# --------------------------------------------------------------------------- + +test_that(".resolve_url appends a missing trailing slash", { + withr::local_envvar(USCOGDATA_URL = "https://example.org/s/TOKEN/download") + expect_equal(.resolve_url(), "https://example.org/s/TOKEN/download/") +}) + +test_that(".resolve_url leaves an existing trailing slash alone", { + withr::local_envvar(USCOGDATA_URL = "https://example.org/s/TOKEN/download/") + expect_equal(.resolve_url(), "https://example.org/s/TOKEN/download/") +}) + +test_that(".resolve_url normalizes a local path without a trailing slash", { + withr::local_envvar(USCOGDATA_URL = "/tmp/corpus") + expect_equal(.resolve_url(), "/tmp/corpus/") +}) + +test_that(".resolve_url does not invent a slash for an empty setting", { + # An unset/empty URL must stay empty so the "not configured" guard in + # manifest.R still fires, rather than degrading into a bare "/" root. + withr::local_envvar(USCOGDATA_URL = "") + withr::local_options(uscogdata.url = "") + expect_equal(.resolve_url(), "") +}) -- 2.54.0