From 62cfbe8e48b82ccd9d3760424273477a62259431 Mon Sep 17 00:00:00 2001 From: Joel Natividad <1980690+jqnatividad@users.noreply.github.com> Date: Thu, 8 Oct 2026 15:48:02 -0400 Subject: [PATCH 1/5] fix(pii): ship the default PII regexes in the package and search them by absolute path The default regex file was moved to the repo root in 2023, but the screening code looks for it next to pii_screening.py, so quick screening always failed with "No such file or directory". Full screening passed the bare file name instead, which only resolved when the worker's current directory happened to be the repo root, and with a custom regex resource it passed the URL's basename rather than the downloaded file. Move the file into ckanext/datapusher_plus/, include it in the package data, and have both modes search the same absolute path. Co-Authored-By: Claude Opus 5.5 --- .serena/memories/codebase_structure.md | 1 - MANIFEST.in | 2 +- README.md | 2 +- .../datapusher_plus/default-pii-regexes.txt | 0 ckanext/datapusher_plus/pii_screening.py | 4 +++- 5 files changed, 5 insertions(+), 4 deletions(-) rename default-pii-regexes.txt => ckanext/datapusher_plus/default-pii-regexes.txt (100%) diff --git a/.serena/memories/codebase_structure.md b/.serena/memories/codebase_structure.md index 58de9bb3..dd3c4316 100644 --- a/.serena/memories/codebase_structure.md +++ b/.serena/memories/codebase_structure.md @@ -19,7 +19,6 @@ ├── setup.py / setup.cfg / MANIFEST.in ├── requirements.txt / requirements-dev.txt ├── dot-env.template # Sample env vars -├── default-pii-regexes.txt # Default PII regex patterns ├── test_config.py # Tests configuration helper ├── wsgi.py # WSGI entry shim ├── .coveragerc # source = ckanext/datapusher_plus (fixed in PR/branch diff --git a/MANIFEST.in b/MANIFEST.in index 70cc7f29..0bbf6329 100644 --- a/MANIFEST.in +++ b/MANIFEST.in @@ -1,5 +1,5 @@ include README.md include LICENSE include requirements.txt -recursive-include ckanext/datapusher_plus *.html *.json *.js *.less *.css *.mo *.yaml +recursive-include ckanext/datapusher_plus *.html *.json *.js *.less *.css *.mo *.yaml default-pii-regexes.txt recursive-include ckanext/datapusher_plus/migration *.ini *.py *.mako diff --git a/README.md b/README.md index 0743fdde..fc1930f4 100644 --- a/README.md +++ b/README.md @@ -126,7 +126,7 @@ It features: * auto-index columns based on its cardinality/format (unique indices created for columns with all unique values, auto-index columns whose cardinality is below a given threshold; auto-index date columns) * check for duplicates, and optionally deduplicate rows * optionally screen for Personally Identifiable Information (PII), with an option to "quarantine" the PII-candidate rows in a separate resource, while still creating the screened resource. - * optional ability to specify a custom PII screening regex set, instead of the [default PII screening regex set](https://github.com/dathere/datapusher-plus/blob/master/default-pii-regexes.txt). + * optional ability to specify a custom PII screening regex set, instead of the [default PII screening regex set](https://github.com/dathere/datapusher-plus/blob/main/ckanext/datapusher_plus/default-pii-regexes.txt). Even with all these pre-processing tasks, qsv typically takes less than 5 seconds to finish all its analysis tasks, even for a 100mb CSV file. diff --git a/default-pii-regexes.txt b/ckanext/datapusher_plus/default-pii-regexes.txt similarity index 100% rename from default-pii-regexes.txt rename to ckanext/datapusher_plus/default-pii-regexes.txt diff --git a/ckanext/datapusher_plus/pii_screening.py b/ckanext/datapusher_plus/pii_screening.py index 7efe062c..ce35e13d 100644 --- a/ckanext/datapusher_plus/pii_screening.py +++ b/ckanext/datapusher_plus/pii_screening.py @@ -70,6 +70,8 @@ def screen_for_pii( # Count of PII candidate matches, surfaced to the caller so the # v3.0 PII-review suspend gate has a real number to threshold on. pii_candidate_count = 0 + # Both modes search the same file. It must be an absolute path: the + # worker's current directory is not the package directory. pii_regex_fname = p.absolute() if conf.PII_QUICK_SCREEN: @@ -94,7 +96,7 @@ def screen_for_pii( qsv_searchset_csv = os.path.join(temp_dir, "qsv_searchset.csv") try: qsv_searchset = qsv.searchset( - pii_regex_file, + pii_regex_fname, tmp, ignore_case=True, flag="PII_info", From 2140e84d1281de61471ad2f0d40e05325de1876d Mon Sep 17 00:00:00 2001 From: Joel Natividad <1980690+jqnatividad@users.noreply.github.com> Date: Thu, 8 Oct 2026 15:50:35 -0400 Subject: [PATCH 2/5] fix(pii): a quick PII screen of a file with no PII no longer fails the job `qsv searchset --quick` exits 1 when nothing matches, and the quick screen treated that like any failure, so with pii_quick_screen enabled every file without PII failed with "Cannot quickly search CSV for PII". Run it with check=False and read the exit code: 0 is a match (its row number is on stderr); 1 with no error message (or, with QSV_ERROR_FORMAT=json, a `no_match` error) is no PII; anything else is still an error. `--not-one` is no help: with it, qsv's sequential search prints the row count to stderr when nothing matched, which reads as a match. Co-Authored-By: Claude Opus 5.5 --- ckanext/datapusher_plus/pii_screening.py | 33 ++++++++++++++++++++++-- ckanext/datapusher_plus/qsv_utils.py | 6 ++++- 2 files changed, 36 insertions(+), 3 deletions(-) diff --git a/ckanext/datapusher_plus/pii_screening.py b/ckanext/datapusher_plus/pii_screening.py index ce35e13d..0ceb9a2b 100644 --- a/ckanext/datapusher_plus/pii_screening.py +++ b/ckanext/datapusher_plus/pii_screening.py @@ -15,6 +15,25 @@ from ckanext.datapusher_plus.qsv_utils import QSVCommand +def _is_searchset_no_match(result) -> bool: + """True if a ``qsv searchset`` run failed only because nothing matched. + + qsv exits 1 for "no match" and prints nothing, or, when asked for + structured errors (``QSV_ERROR_FORMAT=json``), a single JSON error line + of kind ``no_match``. Any other failure prints an error message. + """ + if result.returncode != 1: + return False + stderr = (result.stderr or "").strip() + if not stderr: + return True + try: + err = json.loads(stderr.splitlines()[-1]).get("error", {}) + except (ValueError, AttributeError): + return False + return isinstance(err, dict) and err.get("kind") == "no_match" + + def screen_for_pii( tmp: str, resource: dict, @@ -82,14 +101,24 @@ def screen_for_pii( tmp, ignore_case=True, quick=True, + check=False, ) except utils.JobError as e: raise utils.JobError("Cannot quickly search CSV for PII: %s", e) - pii_candidate_row = str(qsv_searchset.stderr) - if pii_candidate_row: + # Exit 0: a match, with its row number on stderr. Exit 1 with no + # error message: no match, so no PII. (``--not-one`` can't be used: + # with it, qsv prints the row count to stderr even when nothing + # matched, which reads as a match.) + pii_candidate_row = "" + if qsv_searchset.returncode == 0: + pii_candidate_row = str(qsv_searchset.stderr) pii_found = True # Quick screen only detects presence, not a count. pii_candidate_count = 1 + elif not _is_searchset_no_match(qsv_searchset): + raise utils.JobError( + f"Cannot quickly search CSV for PII: {qsv_searchset.stderr}" + ) else: logger.info("Scanning for PII using %s...", pii_regex_file) diff --git a/ckanext/datapusher_plus/qsv_utils.py b/ckanext/datapusher_plus/qsv_utils.py index 8029b32a..44249209 100644 --- a/ckanext/datapusher_plus/qsv_utils.py +++ b/ckanext/datapusher_plus/qsv_utils.py @@ -847,6 +847,7 @@ def searchset( flag_matches_only: bool = False, json_output: bool = False, output_file: Optional[str] = None, + check: bool = True, ) -> subprocess.CompletedProcess: """ Search a CSV file for patterns defined in a regex file. @@ -860,6 +861,9 @@ def searchset( flag_matches_only: Whether to only output matching rows json_output: Whether to output JSON output_file: Path to the output file + check: Raise on a non-zero exit. qsv exits 1 when nothing + matches (outside JSON mode), so callers that treat "no + match" as a result pass False and read ``returncode``. Returns: The result of the command @@ -887,4 +891,4 @@ def searchset( if output_file: args.extend(["--output", output_file]) - return self._run_command(args) + return self._run_command(args, check=check) From 5447ae42834e47c4e46d567dedb63f926afce470 Mon Sep 17 00:00:00 2001 From: Joel Natividad <1980690+jqnatividad@users.noreply.github.com> Date: Thu, 8 Oct 2026 15:50:46 -0400 Subject: [PATCH 3/5] fix(pii): PII screening errors raise JobError instead of TypeError Seven JobError calls in pii_screening.py passed logging-style arguments ("... %s", e), but JobError takes a single message, so each of those paths raised TypeError instead: a failed search, the "PII candidate found, job aborted" check in both modes, and the PII preview's stats, COPY and connection errors. Format the messages with f-strings. Co-Authored-By: Claude Opus 5.5 --- ckanext/datapusher_plus/pii_screening.py | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/ckanext/datapusher_plus/pii_screening.py b/ckanext/datapusher_plus/pii_screening.py index 0ceb9a2b..70e81673 100644 --- a/ckanext/datapusher_plus/pii_screening.py +++ b/ckanext/datapusher_plus/pii_screening.py @@ -104,7 +104,7 @@ def screen_for_pii( check=False, ) except utils.JobError as e: - raise utils.JobError("Cannot quickly search CSV for PII: %s", e) + raise utils.JobError(f"Cannot quickly search CSV for PII: {e}") # Exit 0: a match, with its row number on stderr. Exit 1 with no # error message: no match, so no PII. (``--not-one`` can't be used: # with it, qsv prints the row count to stderr even when nothing @@ -134,7 +134,7 @@ def screen_for_pii( output_file=qsv_searchset_csv, ) except utils.JobError as e: - raise utils.JobError("Cannot search CSV for PII: %s", e) + raise utils.JobError(f"Cannot search CSV for PII: {e}") pii_json = json.loads(str(qsv_searchset.stderr)) pii_total_matches = int(pii_json["total_matches"]) pii_rows_with_matches = int(pii_json["rows_with_matches"]) @@ -146,14 +146,12 @@ def screen_for_pii( logger.error("PII Candidate/s Found!") if conf.PII_QUICK_SCREEN: raise utils.JobError( - "PII CANDIDATE FOUND on row %s! Job aborted.", - pii_candidate_row.rstrip(), + f"PII CANDIDATE FOUND on row {pii_candidate_row.rstrip()}! Job aborted." ) else: raise utils.JobError( - "PII CANDIDATE/S FOUND! Job aborted. Found %d PII candidate/s in %d row/s.", - pii_total_matches, - pii_rows_with_matches, + f"PII CANDIDATE/S FOUND! Job aborted. Found {pii_total_matches} PII " + f"candidate/s in {pii_rows_with_matches} row/s." ) elif pii_found and conf.PII_SHOW_CANDIDATES and not conf.PII_QUICK_SCREEN: # TODO: Create PII Candidates resource and set package to private if its not private @@ -203,7 +201,7 @@ def screen_for_pii( output_file=None, ) except utils.JobError as e: - raise utils.JobError("Cannot run stats on PII preview CSV: %s", e) + raise utils.JobError(f"Cannot run stats on PII preview CSV: {e}") pii_stats = str(qsv_pii_stats.stdout).strip() pii_stats_dict = [ @@ -254,13 +252,13 @@ def screen_for_pii( try: cur_pii.copy_expert(copy_sql, f) except psycopg2.Error as e: - raise utils.JobError("Postgres COPY failed: %s", e) + raise utils.JobError(f"Postgres COPY failed: {e}") else: pii_copied_count = cur_pii.rowcount raw_connection_pii.commit() except psycopg2.Error as e: - raise utils.JobError("Could not connect to the Datastore: %s", e) + raise utils.JobError(f"Could not connect to the Datastore: {e}") pii_resource["id"] = new_pii_resource_id pii_resource["pii_preview"] = True From 35f23bef0fa3d4e0cd944969a283779a3a8a5c94 Mon Sep 17 00:00:00 2001 From: Joel Natividad <1980690+jqnatividad@users.noreply.github.com> Date: Thu, 8 Oct 2026 15:53:23 -0400 Subject: [PATCH 4/5] test(pii): screen_for_pii against a real qsv with the default regexes Runs both modes from a directory that isn't the repo root, covering a file without PII, aborting on PII (quick: the row; full: the counts), a failed search raising JobError, and the no-match detection, including the structured `no_match` error qsv-client will request. Co-Authored-By: Claude Opus 5.5 --- tests/test_pii_screening_search.py | 135 +++++++++++++++++++++++++++++ 1 file changed, 135 insertions(+) create mode 100644 tests/test_pii_screening_search.py diff --git a/tests/test_pii_screening_search.py b/tests/test_pii_screening_search.py new file mode 100644 index 00000000..f9361f45 --- /dev/null +++ b/tests/test_pii_screening_search.py @@ -0,0 +1,135 @@ +# -*- coding: utf-8 -*- +"""``screen_for_pii`` against a real qsv, with the default regex set. + +Regression coverage for three bugs that together kept PII screening from +working with its default configuration: + +* the default regex file lived at the repo root while the code looked for + it next to ``pii_screening.py`` (quick mode always failed), and full mode + passed a bare file name that only resolved when the worker's current + directory was the repo root; +* ``searchset --quick`` exits 1 when nothing matches, so a quick screen of + a file with no PII failed the job; +* ``JobError`` was called with logging-style arguments, which raised + ``TypeError`` instead. + +The tests run from a temporary directory, as a worker would, so a relative +regex path cannot pass by accident. +""" + +import os +import shutil +from pathlib import Path +from unittest import mock + +import pytest + + +def _locate_qsv(): + for c in (os.environ.get("QSV_BIN"), shutil.which("qsvdp"), shutil.which("qsv")): + if c and Path(c).is_file(): + return c + return None + + +QSV_BIN = _locate_qsv() +requires_qsv = pytest.mark.skipif(QSV_BIN is None, reason="needs a qsv binary (QSV_BIN)") + + +def test_default_regex_file_ships_next_to_the_module(): + pytest.importorskip("ckan") + from ckanext.datapusher_plus import pii_screening + + assert Path(pii_screening.__file__).with_name("default-pii-regexes.txt").is_file() + + +@pytest.fixture +def screen(tmp_path, monkeypatch): + """Run ``screen_for_pii`` on CSV text, in quick or full mode.""" + pytest.importorskip("ckan") + from ckanext.datapusher_plus import pii_screening, qsv_utils + + monkeypatch.chdir(tmp_path) + + def run(csv_text, quick): + csv_path = tmp_path / "data.csv" + csv_path.write_text(csv_text) + conf = pii_screening.conf + with mock.patch.object(qsv_utils.conf, "QSV_BIN", Path(QSV_BIN)), \ + mock.patch.object(conf, "PII_REGEX_RESOURCE_ID", None), \ + mock.patch.object(conf, "PII_QUICK_SCREEN", quick), \ + mock.patch.object(conf, "PII_FOUND_ABORT", True), \ + mock.patch.object(conf, "PII_SHOW_CANDIDATES", False): + qsv = qsv_utils.QSVCommand(logger=mock.Mock()) + return pii_screening.screen_for_pii( + str(csv_path), {"id": "res"}, qsv, str(tmp_path), mock.Mock() + ) + + return run + + +CLEAN = "name,city\nAlice,NYC\nBob,LA\n" +WITH_SSN = "name,ssn\nAlice,none\nBob,123-45-6789\n" + + +@requires_qsv +@pytest.mark.parametrize("quick", [True, False], ids=["quick", "full"]) +def test_a_file_without_pii_passes(screen, quick): + assert screen(CLEAN, quick) == (False, 0) + + +@requires_qsv +def test_quick_screen_aborts_on_pii_with_the_row(screen): + from ckanext.datapusher_plus import utils + + with pytest.raises(utils.JobError, match=r"PII CANDIDATE FOUND on row \d+! Job aborted\."): + screen(WITH_SSN, quick=True) + + +@requires_qsv +def test_full_screen_aborts_on_pii_with_the_counts(screen): + from ckanext.datapusher_plus import utils + + with pytest.raises( + utils.JobError, match=r"Found 1 PII candidate/s in 1 row/s\." + ): + screen(WITH_SSN, quick=False) + + +@requires_qsv +@pytest.mark.parametrize( + "quick, message", + [(True, "Cannot quickly search CSV for PII"), (False, "Cannot search CSV for PII")], + ids=["quick", "full"], +) +def test_a_failed_search_raises_job_error(screen, quick, message, tmp_path): + from ckanext.datapusher_plus import pii_screening, utils + + # a regex file qsv cannot read makes searchset itself fail + missing = tmp_path / "missing-regexes.txt" + with mock.patch.object(pii_screening.Path, "absolute", return_value=missing), \ + pytest.raises(utils.JobError, match=message): + screen(CLEAN, quick) + + +@pytest.mark.parametrize( + "returncode, stderr, expected", + [ + (1, "", True), # qsv's text mode prints nothing for "no match" + (1, "\n", True), + (1, '{"error":{"kind":"no_match","level":"error","message":"","exit_code":1}}\n', True), + (1, 'a warning\n{"error":{"kind":"no_match","exit_code":1}}', True), + (1, '{"error":{"kind":"io","message":"No such file","exit_code":1}}', False), + (1, "io error: No such file or directory (os error 2)", False), + (0, "", False), # a success is not a "no match" + (2, "", False), + ], +) +def test_is_searchset_no_match(returncode, stderr, expected): + pytest.importorskip("ckan") + import subprocess + + from ckanext.datapusher_plus.pii_screening import _is_searchset_no_match + + result = subprocess.CompletedProcess(args=[], returncode=returncode, stdout="", stderr=stderr) + assert _is_searchset_no_match(result) is expected From 3354b8d3a28867e0804517f3faf854326380f71e Mon Sep 17 00:00:00 2001 From: Joel Natividad <1980690+jqnatividad@users.noreply.github.com> Date: Thu, 8 Oct 2026 16:20:46 -0400 Subject: [PATCH 5/5] fix(pii): fail clearly when the custom PII regex resource isn't found When ckanext.datapusher_plus.pii_regex_resource_id_or_alias named a resource that isn't in the DataStore, the regex path was never set and screen_for_pii crashed with UnboundLocalError. Raise a JobError that names the resource and the setting instead. Co-Authored-By: Claude Opus 5.5 --- ckanext/datapusher_plus/pii_screening.py | 6 ++++++ tests/test_pii_screening_search.py | 14 ++++++++++++++ 2 files changed, 20 insertions(+) diff --git a/ckanext/datapusher_plus/pii_screening.py b/ckanext/datapusher_plus/pii_screening.py index 70e81673..6950795f 100644 --- a/ckanext/datapusher_plus/pii_screening.py +++ b/ckanext/datapusher_plus/pii_screening.py @@ -68,6 +68,12 @@ def screen_for_pii( pii_regex_resource_exist = dsu.datastore_resource_exists( conf.PII_REGEX_RESOURCE_ID ) + if not pii_regex_resource_exist: + raise utils.JobError( + f"PII regex resource {conf.PII_REGEX_RESOURCE_ID!r} not found in the " + "DataStore. Check ckanext.datapusher_plus.pii_regex_resource_id_or_alias, " + "or unset it to use the default PII regexes." + ) if pii_regex_resource_exist: pii_resource = dsu.get_resource(conf.PII_REGEX_RESOURCE_ID) pii_regex_url = pii_resource["url"] diff --git a/tests/test_pii_screening_search.py b/tests/test_pii_screening_search.py index f9361f45..4c739689 100644 --- a/tests/test_pii_screening_search.py +++ b/tests/test_pii_screening_search.py @@ -133,3 +133,17 @@ def test_is_searchset_no_match(returncode, stderr, expected): result = subprocess.CompletedProcess(args=[], returncode=returncode, stdout="", stderr=stderr) assert _is_searchset_no_match(result) is expected + + +def test_a_missing_custom_regex_resource_is_a_clear_job_error(tmp_path): + """A configured regex resource that isn't in the DataStore used to leave + the regex path unbound and crash with NameError.""" + pytest.importorskip("ckan") + from ckanext.datapusher_plus import pii_screening, utils + + with mock.patch.object(pii_screening.conf, "PII_REGEX_RESOURCE_ID", "no-such-resource"), \ + mock.patch.object(pii_screening.dsu, "datastore_resource_exists", return_value=None), \ + pytest.raises(utils.JobError, match="PII regex resource 'no-such-resource' not found"): + pii_screening.screen_for_pii( + str(tmp_path / "data.csv"), {"id": "res"}, mock.Mock(), str(tmp_path), mock.Mock() + )