-
Notifications
You must be signed in to change notification settings - Fork 33
fix: correct pii_screening config-key typo + documentation-audit findings #324
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,183 @@ | ||
| # -*- coding: utf-8 -*- | ||
| """ | ||
| Regression coverage for the ``pii_screening`` config-key typo fix. | ||
|
|
||
| A documentation audit (2026-05-19) found that ``config.py`` read | ||
| ``PII_SCREENING`` from ``ckanext.datastore_plus.pii_screening`` — a | ||
| typo: ``datastore_plus`` instead of ``datapusher_plus``. Every other | ||
| DP+ setting, the ``config_declaration.yaml`` declaration, and the | ||
| README all use the ``datapusher_plus`` namespace. The practical | ||
| effect of the typo: operators enabling PII screening via the | ||
| documented key ``ckanext.datapusher_plus.pii_screening`` had no | ||
| effect — ``PII_SCREENING`` silently stayed at its ``False`` fallback, | ||
| so the PII-screening feature could not be turned on as documented. | ||
|
|
||
| These tests pin three properties: | ||
|
|
||
| 1. The documented key ``ckanext.datapusher_plus.pii_screening`` | ||
| actually drives ``PII_SCREENING`` — the functional fix. | ||
| 2. The old typo'd key ``ckanext.datastore_plus.pii_screening`` does | ||
| NOT drive it — proves the read moved off the wrong namespace and | ||
| would catch a regression that reintroduced the typo. | ||
| 3. The key string read in ``config.py`` matches the one declared in | ||
| ``config_declaration.yaml`` — a #179-style declaration-vs-code | ||
| drift guard (AST-parsed to avoid the CKAN bootstrap). | ||
| """ | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| from pathlib import Path | ||
|
|
||
| import pytest | ||
|
|
||
|
|
||
| REPO_ROOT = Path(__file__).resolve().parent.parent | ||
|
|
||
|
|
||
| @pytest.fixture(autouse=True) | ||
| def _reset_config_module(): | ||
| """Reload ``ckanext.datapusher_plus.config`` after each test so a | ||
| ``_reload_config`` call here can't leak a monkeypatched config | ||
| snapshot into later tests via ``sys.modules``. | ||
|
|
||
| ``importlib.reload`` mutates the module in place; without this | ||
| teardown the last reload's constants would persist for the rest | ||
| of the session, making unrelated tests order-dependent. | ||
|
|
||
| As an autouse fixture this is set up before the test's | ||
| ``monkeypatch`` fixture, so its finalizer runs *after* | ||
| ``monkeypatch`` has restored ``tk.config`` — the reload therefore | ||
| recomputes the module constants from the pristine | ||
| test-environment config. | ||
| """ | ||
| yield | ||
| try: | ||
| import importlib | ||
|
|
||
| import ckanext.datapusher_plus.config as conf | ||
|
|
||
| importlib.reload(conf) | ||
| except Exception: | ||
| # A reload failure during teardown must not mask the actual | ||
| # test result; the next test that needs a clean config does | ||
| # its own reload via ``_reload_config`` anyway. | ||
| pass | ||
|
|
||
|
|
||
| def _reload_config(monkeypatch, key, value): | ||
| """Set ``key`` on ``tk.config`` and reload ``config.py`` so its | ||
| import-time ``PII_SCREENING`` constant is recomputed. | ||
|
|
||
| ``importlib.reload`` mutates the module object in place, so the | ||
| recomputed constants would otherwise persist past this test — | ||
| ``monkeypatch`` restores ``tk.config`` but does not re-run | ||
| ``config.py``. The ``_reset_config_module`` autouse fixture below | ||
| reloads the module once more on teardown (after ``tk.config`` is | ||
| restored) so the pristine state is back for later tests regardless | ||
| of suite order. | ||
| """ | ||
| import importlib | ||
|
|
||
| pytest.importorskip("ckan") | ||
| import ckan.plugins.toolkit as tk | ||
|
|
||
| monkeypatch.setitem(tk.config, key, value) | ||
| import ckanext.datapusher_plus.config as conf | ||
|
|
||
| return importlib.reload(conf) | ||
|
|
||
|
|
||
| def test_documented_key_enables_pii_screening(monkeypatch): | ||
| # The core fix: the key the README + config_declaration.yaml | ||
| # document must actually flip PII_SCREENING on. | ||
| conf = _reload_config( | ||
| monkeypatch, "ckanext.datapusher_plus.pii_screening", True | ||
| ) | ||
| assert conf.PII_SCREENING is True | ||
|
|
||
|
|
||
| def test_old_typo_key_has_no_effect(monkeypatch): | ||
| # The pre-fix typo'd key must NOT drive PII_SCREENING anymore. | ||
| # Set ONLY the typo'd key to True, with the correct key absent, | ||
| # and PII_SCREENING must stay at its False default. A regression | ||
| # that reinstated ``datastore_plus`` would flip this to True. | ||
| pytest.importorskip("ckan") | ||
| import ckan.plugins.toolkit as tk | ||
|
|
||
| # Ensure the correct key is absent so the assertion is unambiguous | ||
| # (config_declaration.yaml declares it with default false, so the | ||
| # test env may have it pre-populated). | ||
| monkeypatch.delitem( | ||
| tk.config, "ckanext.datapusher_plus.pii_screening", raising=False | ||
| ) | ||
| conf = _reload_config( | ||
| monkeypatch, "ckanext.datastore_plus.pii_screening", True | ||
| ) | ||
| assert conf.PII_SCREENING is False | ||
|
|
||
|
|
||
| def test_config_py_key_matches_declaration(): | ||
| """The key string read in ``config.py`` must match the one | ||
| declared in ``config_declaration.yaml``. A namespace typo in | ||
| either source is exactly the bug this test exists to catch.""" | ||
| import ast | ||
|
|
||
| # PyYAML guarded the same way the #112 / #142 / #61 drift-guards | ||
| # are — degrade to a skip in dep-light CI rather than ImportError. | ||
| try: | ||
| import yaml | ||
| except ImportError: | ||
| pytest.skip("PyYAML not available") | ||
|
|
||
| # config_declaration.yaml side — iterate all groups/options so the | ||
| # guard survives a future declaration restructure. | ||
| yaml_path = ( | ||
| REPO_ROOT | ||
| / "ckanext" | ||
| / "datapusher_plus" | ||
| / "config_declaration.yaml" | ||
| ) | ||
| decl = yaml.safe_load(yaml_path.read_text(encoding="utf-8")) | ||
| declared_keys = { | ||
| opt.get("key") | ||
| for group in decl.get("groups", []) | ||
| for opt in group.get("options", []) | ||
| } | ||
| assert "ckanext.datapusher_plus.pii_screening" in declared_keys, ( | ||
| "config_declaration.yaml must declare " | ||
| "ckanext.datapusher_plus.pii_screening." | ||
| ) | ||
|
|
||
| # config.py side — AST-find the PII_SCREENING assignment and the | ||
| # config key string it reads. | ||
| config_py = ( | ||
| REPO_ROOT / "ckanext" / "datapusher_plus" / "config.py" | ||
| ).read_text() | ||
| tree = ast.parse(config_py) | ||
| found_key = None | ||
| for node in ast.walk(tree): | ||
| if not ( | ||
| isinstance(node, ast.Assign) | ||
| and any( | ||
| getattr(t, "id", None) == "PII_SCREENING" | ||
| for t in node.targets | ||
| ) | ||
| ): | ||
| continue | ||
| for call in ast.walk(node): | ||
| if ( | ||
| isinstance(call, ast.Call) | ||
| and getattr(call.func, "attr", None) == "get" | ||
| and call.args | ||
| and isinstance(call.args[0], ast.Constant) | ||
| ): | ||
| found_key = call.args[0].value | ||
| break | ||
| break | ||
| assert found_key == "ckanext.datapusher_plus.pii_screening", ( | ||
| f"config.py reads PII_SCREENING from {found_key!r}; must be " | ||
| "'ckanext.datapusher_plus.pii_screening' to match the " | ||
| "config_declaration.yaml declaration and the README " | ||
| "(the 'datastore_plus' typo was the documented-but-dead " | ||
| "key bug)." | ||
| ) | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.