jared
748ca4a56e
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).
67 lines
2.8 KiB
R
67 lines
2.8 KiB
R
test_that(".cfg resolves defaults, options, and env vars in priority order", {
|
|
withr::with_options(list(uscogdata.manifest_ttl_secs = NULL), {
|
|
withr::with_envvar(c(USCOGDATA_MANIFEST_TTL_SECS = NA), {
|
|
expect_equal(uscogdata:::.cfg("manifest_ttl_secs"), 3600L)
|
|
})
|
|
})
|
|
|
|
withr::with_options(list(uscogdata.url = "https://opt.example/"), {
|
|
withr::with_envvar(c(USCOGDATA_URL = NA), {
|
|
expect_equal(uscogdata:::.cfg("url"), "https://opt.example/")
|
|
})
|
|
})
|
|
|
|
withr::with_envvar(c(USCOGDATA_URL = "https://env.example/"), {
|
|
withr::with_options(list(uscogdata.url = "https://opt.example/"), {
|
|
expect_equal(uscogdata:::.cfg("url"), "https://env.example/")
|
|
})
|
|
})
|
|
})
|
|
|
|
test_that(".resolve_cache_dir falls back to R_user_dir", {
|
|
withr::with_envvar(c(USCOGDATA_CACHE_DIR = NA), {
|
|
withr::with_options(list(uscogdata.cache_dir = NULL), {
|
|
expect_equal(uscogdata:::.resolve_cache_dir(),
|
|
tools::R_user_dir("uscogdata", "cache"))
|
|
})
|
|
})
|
|
})
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# 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(), "")
|
|
})
|