fix(pii): make PII screening work with its default regexes

.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

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

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.
 

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,
@@ -49,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"]
@@ -70,6 +95,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:
@@ -80,21 +107,31 @@ 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:
+            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
+        # 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)
         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",
@@ -103,7 +140,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"])
@@ -115,14 +152,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
@@ -172,7 +207,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 = [
@@ -223,13 +258,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

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)

tests/test_pii_screening_search.py

@@ -0,0 +1,149 @@
+# -*- 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
+
+
+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()
+        )