fix(#3): normalize the corpus URL's trailing slash at resolution
R-CMD-check / check (push) Successful in 2m42s
R-CMD-check / check (pull_request) Successful in 2m33s

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. <html> <head>`. 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 `"<url-or-local-path>/"`. 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).
This commit is contained in:
2026-07-25 18:37:42 -04:00
parent fa40266d07
commit 748ca4a56e
2 changed files with 62 additions and 1 deletions
+24 -1
View File
@@ -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")
+38
View File
@@ -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(), "")
})