From efc0bd16b1e16877e40fa2e61d628d9c772db390 Mon Sep 17 00:00:00 2001 From: Jared Knowles Date: Tue, 28 Apr 2026 14:16:37 -0400 Subject: [PATCH] fix(search): soft-fail on per-row excluded type and malformed regex name Cross-task review found two edge cases that violated the basket-mode soft-fail contract: - Per-row excluded type (e.g. type = c(NA, "special_district")) hit .coerce_type()'s abort inside the per-row resolver, killing the whole basket call. Now treated as no_match in the sidecar. - Malformed regex in the substring fallback (e.g. name = "San(Diego") propagated DuckDB engine errors. .escape_regex() now backslash- escapes meta characters before the regexp_matches call. Utility- mode regex behavior is unchanged. Plus a new public-surface test for the all-no-match case. --- R/basket.R | 5 ++- R/search.R | 23 +++++++++++++- tests/testthat/test-search.R | 60 ++++++++++++++++++++++++++++++++++++ 3 files changed, 86 insertions(+), 2 deletions(-) diff --git a/R/basket.R b/R/basket.R index bca6985..9f737e4 100644 --- a/R/basket.R +++ b/R/basket.R @@ -39,9 +39,12 @@ } # Convert a type input (integer-like or label) into the canonical label -# string used in the sidecar query_type column. +# string used in the sidecar query_type column. Excluded types (4/5 / +# special_district / school_district) are returned as-is so the sidecar +# records what the user passed without calling .coerce_type() (which aborts). #' @noRd .type_to_label <- function(type) { + if (.is_excluded_type(type)) return(as.character(type)) int_type <- .coerce_type(type) unname(c("0" = "state", "1" = "county", "2" = "city", "3" = "township")[[as.character(int_type)]]) } diff --git a/R/search.R b/R/search.R index b0b7da9..d2b78d6 100644 --- a/R/search.R +++ b/R/search.R @@ -132,6 +132,14 @@ cog_gov_search <- function(name = NULL, state = NULL, type = NULL) { ) } +#' @noRd +.escape_regex <- function(x) { + # Backslash-escape POSIX regex metacharacters so `name` is treated as a + # literal substring in the DuckDB regexp_matches call (substring fallback + # only; utility-mode intentionally preserves regex behavior). + gsub("([\\^$.|?*+(){}\\[\\]])", "\\\\\\1", x, perl = TRUE) +} + #' @noRd .is_excluded_type <- function(type) { excluded <- c("4", "5", "special_district", "school_district") @@ -245,6 +253,19 @@ cog_gov_search <- function(name = NULL, state = NULL, type = NULL) { )) } + # Short-circuit: excluded type (4/5 / special_district / school_district) + # -> no_match without SQL, preserving soft-fail contract. + if (!is.na(type) && .is_excluded_type(type)) { + empty <- .empty_xwalk_tibble() + return(list( + status = "no_match", + match_method = NA_character_, + n_candidates = 0L, + row = empty, + candidates = empty + )) + } + preds <- character(0) if (!is.na(state)) { st_fips <- .coerce_state_to_fips(state) @@ -282,7 +303,7 @@ cog_gov_search <- function(name = NULL, state = NULL, type = NULL) { "SELECT * FROM canonical_fips_xwalk", base_where, conj, - sprintf("regexp_matches(gov_name, %s, 'i')", .sql_lit_chr(name)) + sprintf("regexp_matches(gov_name, %s, 'i')", .sql_lit_chr(.escape_regex(name))) ) sub <- tibble::as_tibble(DBI::dbGetQuery(con, sub_sql)) diff --git a/tests/testthat/test-search.R b/tests/testthat/test-search.R index ef2a892..07bdb7e 100644 --- a/tests/testthat/test-search.R +++ b/tests/testthat/test-search.R @@ -375,3 +375,63 @@ test_that("cog_gov_search basket mode message points to the sidecar accessor", { regexp = "cog_basket_resolution" ) }) + +# ---- F1: per-row excluded type soft-fail ---- + +test_that(".resolve_basket_row treats excluded type as no_match (not abort)", { + con <- uscogdata:::.ensure_session() + out <- uscogdata:::.resolve_basket_row( + name = "Some District", state = "CA", type = "special_district", con = con + ) + expect_equal(out$status, "no_match") + expect_true(is.na(out$match_method)) + expect_equal(out$n_candidates, 0L) +}) + +test_that("cog_gov_search basket mode skips per-row excluded type without aborting", { + basket <- suppressMessages(cog_gov_search( + name = c("BROWARD COUNTY", "Some District"), + state = c("FL", "FL"), + type = c(NA, "special_district") + )) + # Broward should resolve; the special_district row should be no_match. + expect_equal(nrow(basket), 1L) + expect_equal(basket$canonical_govid, "101006006") + res <- attr(basket, "resolution") + expect_equal(res$status, c("resolved", "no_match")) + # query_type should record what the user passed for the excluded-type row + expect_equal(res$query_type, c(NA_character_, "special_district")) +}) + +# ---- F2: malformed regex name soft-fail ---- + +test_that(".resolve_basket_row treats malformed regex name as no_match", { + con <- uscogdata:::.ensure_session() + # Unbalanced parens would be a regex parse error if not escaped. + out <- uscogdata:::.resolve_basket_row( + name = "San(Diego", state = "CA", type = NA_character_, con = con + ) + expect_equal(out$status, "no_match") +}) + +test_that(".resolve_basket_row escapes regex metacharacters in name", { + con <- uscogdata:::.ensure_session() + # Confirm that names with various metacharacters don't error. + expect_no_error(uscogdata:::.resolve_basket_row( + name = "Foo*Bar+Baz", state = "FL", type = NA_character_, con = con + )) +}) + +# ---- F3: all-no-match basket public surface ---- + +test_that("cog_gov_search basket all-no-match returns 0-row tibble with full sidecar", { + basket <- suppressMessages(cog_gov_search( + name = c("Notarealplace1", "Notarealplace2"), + state = c("NY", "CA") + )) + expect_equal(nrow(basket), 0L) + expect_true("canonical_govid" %in% names(basket)) + res <- attr(basket, "resolution") + expect_equal(nrow(res), 2L) + expect_true(all(res$status == "no_match")) +})