diff --git a/DESCRIPTION b/DESCRIPTION index 9447044..f8fa1f1 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -1,6 +1,6 @@ Package: datadiff Title: Data Validation Based on 'YAML' Rules -Version: 0.4.9 +Version: 0.4.9.9000 Authors@R: c( person("Vincent", "Guyader", , "vincent@thinkr.fr", role = c("cre", "aut"), comment = c(ORCID = "0000-0003-0671-9270")), diff --git a/NEWS.md b/NEWS.md index b36e3b8..7601979 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,3 +1,20 @@ +# datadiff (development version) + +## Breaking changes + +* `compare_datasets_from_yaml()` now raises an explicit error when one or more + key columns are absent from either dataset, naming the missing column(s) and + the dataset(s) concerned, on every code path (with or without an explicit + YAML rules file). It previously emitted a vague message + ("could not find key in both data") and returned a truncated 6-field list + (no `coverage`, no `summary`) that broke downstream consumers such as + `datadiff_report_html()` (issue #16). + +* `compare_datasets_from_yaml()` rejects an empty (`character(0)`) or + non-character `key` with a clear error. An empty key previously fell through + to a keyless cross join (deprecated dplyr behavior producing a cartesian + product). + # datadiff 0.4.9 ## Bug fixes diff --git a/R/compare_datasets_from_yaml.R b/R/compare_datasets_from_yaml.R index 65c9f1b..ee74ba9 100644 --- a/R/compare_datasets_from_yaml.R +++ b/R/compare_datasets_from_yaml.R @@ -111,6 +111,49 @@ read_rules <- function(path) { r } +#' Validate a comparison key against both datasets +#' +#' Shared guard for every code path that consumes a key: checks the type and +#' emptiness of the key, then its presence in both datasets, and raises an +#' explicit error naming the missing column(s) and the dataset(s) concerned. +#' +#' @param key Character vector of key column names. +#' @param ref_cols Column names of the reference dataset. +#' @param cand_cols Column names of the candidate dataset. +#' @return \code{NULL}, invisibly. Called for its side effect (error). +#' @noRd +validate_comparison_key <- function(key, ref_cols, cand_cols) { + if (!is.character(key) || length(key) == 0) { + stop( + "Parameter 'key' must be a non-empty character vector specifying column name(s) to use as join key(s).", + call. = FALSE + ) + } + missing_key_ref <- setdiff(key, ref_cols) + missing_key_cand <- setdiff(key, cand_cols) + if (length(missing_key_ref) > 0 || length(missing_key_cand) > 0) { + describe_missing <- function(cols, dataset_name) { + if (length(cols) == 0) { + return(NULL) + } + sprintf( + "%s missing in %s", + paste(sprintf("'%s'", cols), collapse = ", "), + dataset_name + ) + } + details <- c( + describe_missing(missing_key_ref, dataset_name = "data_reference"), + describe_missing(missing_key_cand, dataset_name = "data_candidate") + ) + stop(sprintf( + "Key column(s) not found: %s.", + paste(details, collapse = "; ") + ), call. = FALSE) + } + invisible(NULL) +} + #' Compare datasets using YAML validation rules #' #' Main function for comparing reference and candidate datasets using configurable @@ -119,7 +162,9 @@ read_rules <- function(path) { #' #' @param data_reference Reference dataframe, tibble, or lazy table (tbl_lazy) #' @param data_candidate Candidate dataframe to validate against reference -#' @param key Optional character vector of column names to use as join keys for ordered comparison +#' @param key Optional character vector of column names to use as join keys for +#' ordered comparison. Every key column must exist in both datasets; otherwise +#' an error is raised naming the missing column(s) and the dataset(s) concerned. #' @param path Path to YAML file containing validation rules. If NULL, default rules are #' generated automatically based on the reference dataset structure. #' @param warn_at Warning threshold as fraction of failing tests (default: 1e-14) @@ -264,6 +309,18 @@ compare_datasets_from_yaml <- function(data_reference, data_candidate <- arrow_dataset_to_duckdb(data_candidate, fresh_con, "datadiff_cand") } + # Validate the key argument against BOTH datasets before any other consumer: + # the auto-generated-template path (path = NULL) would otherwise reach + # write_rules_template() first, which validates the reference only and with + # a less specific error. + if (!is.null(key)) { + validate_comparison_key( + key, + ref_cols = get_col_names(data_reference), + cand_cols = get_col_names(data_candidate) + ) + } + # If no path provided, create a temporary YAML with default rules if (is.null(path)) { path <- tempfile(fileext = ".yaml") @@ -403,19 +460,13 @@ compare_datasets_from_yaml <- function(data_reference, row_validation_info <- validate_row_counts(data_reference_p, data_candidate_p, rules) if (!is.null(key)) { - if (isFALSE(all(key %in% get_col_names(data_reference_p))) | isFALSE(all(key %in% get_col_names(data_candidate_p)))) { - message("could not find key in both data") - return( - list( - all_passed = FALSE, - agent = NULL, - reponse = NULL, - missing_in_candidate = NULL, - extra_in_candidate = NULL, - applied_rules = NULL - ) - ) - } + # Re-validated here because the key may come from the YAML rules + # (defaults$keys), which the argument-level check upstream cannot see. + validate_comparison_key( + key, + ref_cols = get_col_names(data_reference_p), + cand_cols = get_col_names(data_candidate_p) + ) # Join candidate to reference on key to handle different row counts cmp <- left_join(data_candidate_p, data_reference_p, by = key, suffix = c("", ref_suffix)) diff --git a/man/compare_datasets_from_yaml.Rd b/man/compare_datasets_from_yaml.Rd index 71bc074..6fc85ab 100644 --- a/man/compare_datasets_from_yaml.Rd +++ b/man/compare_datasets_from_yaml.Rd @@ -29,7 +29,9 @@ compare_datasets_from_yaml( \item{data_candidate}{Candidate dataframe to validate against reference} -\item{key}{Optional character vector of column names to use as join keys for ordered comparison} +\item{key}{Optional character vector of column names to use as join keys for +ordered comparison. Every key column must exist in both datasets; otherwise +an error is raised naming the missing column(s) and the dataset(s) concerned.} \item{path}{Path to YAML file containing validation rules. If NULL, default rules are generated automatically based on the reference dataset structure.} diff --git a/tests/testthat/test-key-parameter.R b/tests/testthat/test-key-parameter.R index 346b5a7..86fb923 100644 --- a/tests/testthat/test-key-parameter.R +++ b/tests/testthat/test-key-parameter.R @@ -72,7 +72,7 @@ by_type: unlink('test_key_null.yaml') }) -test_that("compare_datasets_from_yaml key parameter validation works", { +test_that("compare_datasets_from_yaml errors when key columns are absent", { ref <- data.frame(id = 1:3, value = c(10.0, 20.0, 30.0)) cand <- data.frame(id = 1:3, value = c(10.1, 20.1, 30.1)) @@ -85,17 +85,92 @@ by_type: abs: 0.5 ' - writeLines(yaml_content, 'test_key_validation.yaml') + yaml_path <- tempfile(fileext = ".yaml") + on.exit(unlink(yaml_path), add = TRUE) + writeLines(yaml_content, con = yaml_path) - # Test: Invalid key should return an empty output with all_passed FALSE + # Key absent from both datasets: explicit error naming both datasets + err_both <- tryCatch( + compare_datasets_from_yaml(ref, cand, key = "nonexistent", path = yaml_path), + error = function(e) { + conditionMessage(e) + } + ) + expect_match(err_both, "Key column\\(s\\) not found") + expect_match(err_both, "'nonexistent'", fixed = TRUE) + expect_match(err_both, "data_reference", fixed = TRUE) + expect_match(err_both, "data_candidate", fixed = TRUE) + + # Key present in the reference but absent from the candidate: + # only the candidate is named + cand_no_id <- data.frame(idx = 1:3, value = c(10.1, 20.1, 30.1)) + err_cand <- tryCatch( + compare_datasets_from_yaml(ref, cand_no_id, key = "id", path = yaml_path), + error = function(e) { + conditionMessage(e) + } + ) + expect_match(err_cand, "'id'", fixed = TRUE) + expect_match(err_cand, "data_candidate", fixed = TRUE) + expect_no_match(err_cand, "data_reference", fixed = TRUE) + + # Same scenario without an explicit YAML path (auto-generated template) + expect_error( + compare_datasets_from_yaml(ref, cand_no_id, key = "id"), + regexp = "missing in data_candidate" + ) - res <- compare_datasets_from_yaml(ref, cand, key = "nonexistent", path = 'test_key_validation.yaml') + # Without an explicit YAML path, a key absent from the *reference* must get + # the same explicit error format (not the reference-only error of + # write_rules_template on the auto-generated-template path) + ref_no_id <- data.frame(idx = 1:3, value = c(10.0, 20.0, 30.0)) + cand_id <- data.frame(id = 1:3, value = c(10.1, 20.1, 30.1)) + err_ref <- tryCatch( + compare_datasets_from_yaml(ref_no_id, cand_id, key = "id"), + error = function(e) { + conditionMessage(e) + } + ) + expect_match(err_ref, "'id'", fixed = TRUE) + expect_match(err_ref, "missing in data_reference", fixed = TRUE) + expect_no_match(err_ref, "data_candidate", fixed = TRUE) + + # Multi-column key with a single absent column: only that column is reported + ref_multi <- data.frame(id = 1:3, grp = 1:3, value = 1:3) + cand_multi <- data.frame(id = 1:3, value = 1:3) + err_multi <- tryCatch( + compare_datasets_from_yaml(ref_multi, cand_multi, key = c("id", "grp"), path = yaml_path), + error = function(e) { + conditionMessage(e) + } + ) + expect_match(err_multi, "'grp'", fixed = TRUE) + expect_no_match(err_multi, "'id'", fixed = TRUE) +}) - expect_false(res$all_passed) - expect_null(res$agent) - expect_null(res$reponse) - # Clean up - unlink('test_key_validation.yaml') +test_that("compare_datasets_from_yaml rejects empty or non-character keys", { + ref <- data.frame(id = 1:3, value = c(10.0, 20.0, 30.0)) + cand <- data.frame(id = 1:3, value = c(10.1, 20.1, 30.1)) + + yaml_path <- tempfile(fileext = ".yaml") + on.exit(unlink(yaml_path), add = TRUE) + write_rules_template(ref, key = "id", path = yaml_path) + + # An empty key must not silently fall through to a keyless cross join + expect_error( + compare_datasets_from_yaml(ref, cand, key = character(0), path = yaml_path), + regexp = "non-empty character vector" + ) + expect_error( + compare_datasets_from_yaml(ref, cand, key = character(0)), + regexp = "non-empty character vector" + ) + + # A non-character key gets a clear type error, not a missing-column error + expect_error( + compare_datasets_from_yaml(ref, cand, key = 1L, path = yaml_path), + regexp = "non-empty character vector" + ) }) test_that("key parameter takes precedence over YAML in edge cases", {