From 2167dcfc4f2b66c61ed3998e586bd8e096ff0959 Mon Sep 17 00:00:00 2001 From: Joel Natividad <1980690+jqnatividad@users.noreply.github.com> Date: Wed, 20 May 2026 07:09:47 -0400 Subject: [PATCH 1/5] ci: rewrite test.yml as a real unit-suite job MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The old test.yml ("Tests") was dead 2022-era cruft: a Python 2.7-3.9 matrix, an `actions/checkout@v2` / `setup-python@v2` / `codecov-action@v1` stack, `pip install -r requirements-dev-py2.txt` (that file no longer exists), and `pytest --cov=datapusher` against the pre-extension package name. It cannot have passed since the extension refactor. It also masked a real gap surfaced by the CLAUDE.md audit: NO workflow runs the DP+ unit suite. `ci.yml` runs only the qsv contract regression test + the integration suite; `main.yml` is a manual end-to-end run. Replaced with a `unit-tests` job that runs the full unit suite (`pytest tests/ --ignore=tests/integration`) on push/PR. It mirrors the proven `dpp-test` container setup: the `ckan/ckan-dev:2.11` image, geo system libs, `requirements.txt` + `requirements-dev.txt` + editable install, qsv 20.1.0, and the `PYTEST_DISABLE_PLUGIN_AUTOLOAD` / `QSV_BIN` / `CKAN_INI` env the suite needs. Python 3.10 only: the unit suite imports `ckan.plugins.toolkit`, so it needs a full CKAN install, and the CKAN 2.11 dev image ships Python 3.10. A 3.11-3.13 matrix would require building CKAN from source per version — left as a follow-up. Co-Authored-By: Claude Opus 4.7 (1M context) --- .github/workflows/test.yml | 106 ++++++++++++++++++++++++++++--------- 1 file changed, 82 insertions(+), 24 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index a52c552f..2e2a19ce 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -1,27 +1,85 @@ -name: Tests -on: [push, pull_request] +name: Unit Tests + +on: + push: + branches: [main, dev] + pull_request: + branches: [main] + workflow_dispatch: + +concurrency: + group: dp-unit-${{ github.ref }} + cancel-in-progress: true + +permissions: + contents: read + +env: + QSV_VER: "20.1.0" + jobs: - test: - strategy: - matrix: - python-version: [2.7, 3.6, 3.7, 3.8, 3.9] - fail-fast: false - name: Python ${{ matrix.python-version }} + unit-tests: + name: Unit suite (Python 3.10, CKAN 2.11) runs-on: ubuntu-latest + timeout-minutes: 30 + # The unit suite imports ckan.plugins.toolkit and reads test-core.ini, + # so it needs a full CKAN install. The ckan/ckan-dev:2.11 image ships + # that (Python 3.10). A 3.11-3.13 matrix would mean building CKAN from + # source per version — tracked as a separate follow-up; pyproject + # already classifies 3.10-3.13 support. + container: + image: ckan/ckan-dev:2.11 + options: --user root steps: - - uses: actions/checkout@v2 - - uses: actions/setup-python@v2 - with: - python-version: ${{ matrix.python-version }} - - name: Install requirements (Python 2) - if: ${{ matrix.python-version == '2.7' }} - run: pip install -r requirements-dev-py2.txt - - name: Install requirements (Python 3) - if: ${{ matrix.python-version != '2.7' }} - run: pip install -r requirements-dev.txt - - name: Run tests - run: pytest --cov=datapusher --cov-append --cov-report=xml --disable-warnings tests - - name: Upload coverage report to codecov - uses: codecov/codecov-action@v1 - with: - file: ./coverage.xml + - name: Install system dependencies + run: | + apt-get update -y + apt-get install -y \ + build-essential git wget unzip \ + gdal-bin libgdal-dev libspatialindex-dev libgeos-dev libproj-dev \ + libxslt1-dev libxml2-dev libffi-dev libpq-dev zlib1g-dev + + - name: Checkout datapusher-plus + uses: actions/checkout@v4 + + - name: Install Python dependencies and datapusher-plus + run: | + set -eu + pip install --upgrade pip + # GDAL python binding must match the system libgdal version. + pip install "GDAL==$(gdal-config --version)" + pip install -r requirements.txt -r requirements-dev.txt + pip install -e . + + - name: Install qsv + run: | + set -eu + cd /tmp + QSV_ZIP="qsv-${QSV_VER}-x86_64-unknown-linux-musl.zip" + wget -q "https://github.com/dathere/qsv/releases/download/${QSV_VER}/${QSV_ZIP}" -O qsv.zip + unzip -qo qsv.zip + # Install as /usr/local/bin/qsvdp so QSV_BIN is stable regardless + # of whether the archive ships `qsvdp` or `qsv`. + if [ -f qsvdp ]; then SRC_BIN=qsvdp + elif [ -f qsv ]; then SRC_BIN=qsv + else echo "ERROR: no qsv binary in archive"; ls -la; exit 1; fi + chmod +x "$SRC_BIN" + mv -f "$SRC_BIN" /usr/local/bin/qsvdp + /usr/local/bin/qsvdp --version + + - name: Run unit test suite + env: + # The ckan-dev image's site-packages registers a pytest plugin + # (ckan.tests.pytest_ckan) that calls make_app() in + # pytest_sessionstart — needs a fully-configured CKAN. Disable + # plugin autoload so plain pytest runs. + PYTEST_DISABLE_PLUGIN_AUTOLOAD: "1" + QSV_BIN: /usr/local/bin/qsvdp + CKAN_INI: /srv/app/src/ckan/test-core.ini + run: | + set -eu + # -o addopts= overrides setup.cfg's --pdbcls IPython addopt. + # tests/integration is excluded — it needs the full + # docker-compose stack (Prefect, Postgres, Redis, Solr). + python3 -m pytest tests/ --ignore=tests/integration \ + -o addopts= -p no:cacheprovider -q From 1638bf70ef0ce09bde36adc496a67d1fff08a82e Mon Sep 17 00:00:00 2001 From: Joel Natividad <1980690+jqnatividad@users.noreply.github.com> Date: Wed, 20 May 2026 07:11:17 -0400 Subject: [PATCH 2/5] fix: add xlsm/xlsb to FORMATS + correct spatial-tolerance config-key casing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two config.py correctness fixes surfaced by the documentation audit: * **xlsm/xlsb were ungated.** ``FormatConverterStage`` advertises ``.xlsm`` and ``.xlsb`` support (both in ``SPREADSHEET_EXTENSIONS``), but the ``FORMATS`` default omitted them — so DP+ silently skipped those files before they ever reached the converter. Added both to the ``FORMATS`` default, grouped with the other Excel formats. New ``tests/test_formats_config.py`` (2 tests): xlsm/xlsb are in ``FORMATS``, and a consistency guard that every extension in ``FormatConverterStage.SPREADSHEET_EXTENSIONS`` is gated in by ``FORMATS`` — so a future converter-side addition can't silently leave a format ungated. * **spatial-tolerance config key had inconsistent casing.** ``config.py`` read the relative-tolerance setting from ``ckanext.datapusher_plus.SPATIAL_SIMPLIFICATION_RELATIVE_TOLERANCE`` — UPPERCASE, unlike every other ~50 DP+ config keys. Operators setting the lowercase form (the only form the README ever showed, albeit misspelled) silently had no effect. Lowercased the key string to ``...spatial_simplification_relative_tolerance``. The Python constant name is unchanged; only the ckan.ini key string moved. Co-Authored-By: Claude Opus 4.7 (1M context) --- ckanext/datapusher_plus/config.py | 4 +-- tests/test_formats_config.py | 54 +++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 2 deletions(-) create mode 100644 tests/test_formats_config.py diff --git a/ckanext/datapusher_plus/config.py b/ckanext/datapusher_plus/config.py index 93bab8aa..b76c38ef 100644 --- a/ckanext/datapusher_plus/config.py +++ b/ckanext/datapusher_plus/config.py @@ -40,7 +40,7 @@ # Supported formats FORMATS = tk.config.get( "ckanext.datapusher_plus.formats", - ["csv", "tsv", "tab", "ssv", "xls", "xlsx", "ods", "geojson", "shp", "qgis", "zip"], + ["csv", "tsv", "tab", "ssv", "xls", "xlsx", "xlsm", "xlsb", "ods", "geojson", "shp", "qgis", "zip"], ) if isinstance(FORMATS, str): FORMATS = FORMATS.split() @@ -254,7 +254,7 @@ tk.config.get("ckanext.datapusher_plus.auto_spatial_simplification", True) ) SPATIAL_SIMPLIFICATION_RELATIVE_TOLERANCE = tk.config.get( - "ckanext.datapusher_plus.SPATIAL_SIMPLIFICATION_RELATIVE_TOLERANCE", "0.1" + "ckanext.datapusher_plus.spatial_simplification_relative_tolerance", "0.1" ) # Latitude and longitude column names diff --git a/tests/test_formats_config.py b/tests/test_formats_config.py new file mode 100644 index 00000000..cdcf779f --- /dev/null +++ b/tests/test_formats_config.py @@ -0,0 +1,54 @@ +# -*- coding: utf-8 -*- +""" +Coverage for the ``FORMATS`` config — the gate that decides which +uploaded file formats DataPusher+ will process. + +``format_converter.py`` can convert ``.xlsm`` and ``.xlsb`` workbooks +(both are in ``FormatConverterStage.SPREADSHEET_EXTENSIONS``), but the +``FORMATS`` default in ``config.py`` historically omitted them — so a +``.xlsm`` / ``.xlsb`` resource was silently skipped before it ever +reached the converter. These tests pin: + +1. ``xlsm`` and ``xlsb`` are in the ``FORMATS`` default — the direct + regression guard for that fix. +2. Every spreadsheet extension the converter advertises support for + (``SPREADSHEET_EXTENSIONS``) is present in ``FORMATS`` — a + consistency guard so a future "the converter now also handles + ``.xyz``" change can't silently leave ``.xyz`` ungated. +""" + +from __future__ import annotations + +import pytest + + +@pytest.fixture +def formats(): + pytest.importorskip("ckan") + import ckanext.datapusher_plus.config as conf + + return conf.FORMATS + + +def test_xlsm_and_xlsb_are_in_formats(formats): + # format_converter can convert both; FORMATS must let them in. + assert "xlsm" in formats + assert "xlsb" in formats + + +def test_formats_covers_every_converter_spreadsheet_extension(formats): + # Consistency guard: anything FormatConverterStage advertises in + # SPREADSHEET_EXTENSIONS must be gated in by FORMATS, or DP+ would + # reject a file format it actually knows how to convert. + pytest.importorskip("ckan") + from ckanext.datapusher_plus.jobs.stages.format_converter import ( + FormatConverterStage, + ) + + formats_lower = {f.lower() for f in formats} + for ext in FormatConverterStage.SPREADSHEET_EXTENSIONS: + assert ext.lower() in formats_lower, ( + f"{ext} is in FormatConverterStage.SPREADSHEET_EXTENSIONS but " + f"not in config.FORMATS — DP+ would skip .{ext.lower()} files " + "before the converter ever sees them." + ) From a9e64dbed71fdac5fa0301a00cec484a64e9665d Mon Sep 17 00:00:00 2001 From: Joel Natividad <1980690+jqnatividad@users.noreply.github.com> Date: Wed, 20 May 2026 07:13:07 -0400 Subject: [PATCH 3/5] docs: reconcile README ckan.ini example config blocks to current defaults MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both ckan.ini example blocks were reconciled to DP+'s actual current defaults (the documentation audit found the values had drifted; PR #324 added a stopgap "illustrative, not defaults" note). Each fix verified against config.py and config_declaration.yaml — where a key is declared, the declaration's default is what CKAN 2.10+ applies at runtime, so that is the authoritative value. Per block: * Removed ``use_proxy`` — there is no such config key. DP+ derives proxy use from whether ``download_proxy`` is set (``config.py``: ``USE_PROXY = "...download_proxy" in tk.config``). * ``ssl_verify``: false -> true (the actual default — verifying TLS). * ``preview_rows``: 100 -> 0 (load all rows; matches config_declaration.yaml after PR #324). * ``auto_index_threshold``: 3 -> 10 (issue #142 raised the default). * ``formats``: added ``xlsm xlsb`` (now gated in — see the FORMATS commit in this PR). * ``auto_spatial_simplication`` -> ``auto_spatial_simplification`` and ``spatial_simplication_relative_tolerance`` -> ``spatial_simplification_relative_tolerance`` — the README had always misspelled "simplification"; the corrected lowercase key now matches config.py. * ``longitude_fields``: ``longitude,long,lon`` -> ``longitude,lon`` (the actual default has no ``long``). * ``jinja2_bytecode_cache_dir``: fixed the ``butecode`` typo in the path default. ``chunk_size`` (16384), ``dedup`` (false), ``ignore_file_hash`` (true) and ``auto_alias_unique`` (false) were left as-is — those already match the config_declaration.yaml defaults that win at runtime. (config.py's *inline fallbacks* for those four still drift from the declaration — behavior-preserving dead code, same shape as the preview_rows fallback PR #324 fixed; flagged for a follow-up.) The note above each block was reworded: the block now genuinely lists current defaults, so it is a usable starting-point reference rather than an "illustrative, differs from defaults" disclaimer. Co-Authored-By: Claude Opus 4.7 (1M context) --- README.md | 38 ++++++++++++++++++-------------------- 1 file changed, 18 insertions(+), 20 deletions(-) diff --git a/README.md b/README.md index 2dfa1bf0..bf3c8dc1 100644 --- a/README.md +++ b/README.md @@ -303,23 +303,22 @@ ckan config-tool /etc/ckan/default/ckan.ini "ckanext.datapusher_plus.api_token=$ 7. Add the rest of the DP+ config to your CKAN config (e.g. `/etc/ckan/default/ckan.ini`): -> **Note:** The block below is an illustrative example, not a list of defaults. Several values (e.g. `preview_rows`, `chunk_size`, `dedup`, `auto_index_threshold`, `ignore_file_hash`) differ from DP+'s actual defaults. The authoritative defaults live in [`config_declaration.yaml`](ckanext/datapusher_plus/config_declaration.yaml). Only set the keys you actually want to override. +> **Note:** The block below lists DP+'s settings at their current defaults — copy it as a starting point and change only the keys you need. The authoritative defaults are declared in [`config_declaration.yaml`](ckanext/datapusher_plus/config_declaration.yaml). ```ini # datapusher-plus settings -ckanext.datapusher_plus.use_proxy = false ckanext.datapusher_plus.download_proxy = -ckanext.datapusher_plus.ssl_verify = false +ckanext.datapusher_plus.ssl_verify = true # supports INFO, DEBUG, TRACE - use DEBUG or TRACE when debugging scheming Formulas ckanext.datapusher_plus.upload_log_level = INFO -ckanext.datapusher_plus.formats = csv tsv tab ssv xls xlsx ods geojson shp qgis zip +ckanext.datapusher_plus.formats = csv tsv tab ssv xls xlsx xlsm xlsb ods geojson shp qgis zip ckanext.datapusher_plus.pii_screening = false ckanext.datapusher_plus.pii_found_abort = false ckanext.datapusher_plus.pii_regex_resource_id_or_alias = ckanext.datapusher_plus.pii_show_candidates = false ckanext.datapusher_plus.pii_quick_screen = false ckanext.datapusher_plus.qsv_bin = /usr/local/bin/qsvdp -ckanext.datapusher_plus.preview_rows = 100 +ckanext.datapusher_plus.preview_rows = 0 ckanext.datapusher_plus.download_timeout = 300 ckanext.datapusher_plus.max_content_length = 1256000000000 ckanext.datapusher_plus.chunk_size = 16384 @@ -330,7 +329,7 @@ ckanext.datapusher_plus.unsafe_prefix = unsafe_ ckanext.datapusher_plus.reserved_colnames = _id ckanext.datapusher_plus.prefer_dmy = false ckanext.datapusher_plus.ignore_file_hash = true -ckanext.datapusher_plus.auto_index_threshold = 3 +ckanext.datapusher_plus.auto_index_threshold = 10 ckanext.datapusher_plus.auto_index_dates = true ckanext.datapusher_plus.auto_unique_index = true ckanext.datapusher_plus.summary_stats_options = @@ -343,11 +342,11 @@ ckanext.datapusher_plus.auto_alias = true ckanext.datapusher_plus.auto_alias_unique = false ckanext.datapusher_plus.copy_readbuffer_size = 1048576 ckanext.datapusher_plus.type_mapping = {"String": "text", "Integer": "numeric","Float": "numeric","DateTime": "timestamp","Date": "date","NULL": "text"} -ckanext.datapusher_plus.auto_spatial_simplication = true -ckanext.datapusher_plus.spatial_simplication_relative_tolerance = 0.1 +ckanext.datapusher_plus.auto_spatial_simplification = true +ckanext.datapusher_plus.spatial_simplification_relative_tolerance = 0.1 ckanext.datapusher_plus.latitude_fields = latitude,lat -ckanext.datapusher_plus.longitude_fields = longitude,long,lon -ckanext.datapusher_plus.jinja2_bytecode_cache_dir = /tmp/jinja2_butecode_cache +ckanext.datapusher_plus.longitude_fields = longitude,lon +ckanext.datapusher_plus.jinja2_bytecode_cache_dir = /tmp/jinja2_bytecode_cache ckanext.datapusher_plus.auto_unzip_one_file = true ``` @@ -412,22 +411,21 @@ scheming.dataset_schemas = ckanext.datapusher_plus:dataset-druf.yaml Configure DP+ numerous settings. See [config.py](ckanext/datapusher_plus/config.py) and [`config_declaration.yaml`](ckanext/datapusher_plus/config_declaration.yaml) for details. -> **Note:** The block below is an illustrative example, not a list of defaults. Several values differ from DP+'s actual defaults — see [`config_declaration.yaml`](ckanext/datapusher_plus/config_declaration.yaml) for the authoritative defaults. Only set the keys you actually want to override. +> **Note:** The block below lists DP+'s settings at their current defaults — copy it as a starting point and change only the keys you need. The authoritative defaults are declared in [`config_declaration.yaml`](ckanext/datapusher_plus/config_declaration.yaml). >```ini -> ckanext.datapusher_plus.use_proxy = false > ckanext.datapusher_plus.download_proxy = -> ckanext.datapusher_plus.ssl_verify = false +> ckanext.datapusher_plus.ssl_verify = true > # supports INFO, DEBUG, TRACE - use DEBUG or TRACE when debugging scheming Formulas > ckanext.datapusher_plus.upload_log_level = INFO -> ckanext.datapusher_plus.formats = csv tsv tab ssv xls xlsx ods geojson shp qgis zip +> ckanext.datapusher_plus.formats = csv tsv tab ssv xls xlsx xlsm xlsb ods geojson shp qgis zip > ckanext.datapusher_plus.pii_screening = false > ckanext.datapusher_plus.pii_found_abort = false > ckanext.datapusher_plus.pii_regex_resource_id_or_alias = > ckanext.datapusher_plus.pii_show_candidates = false > ckanext.datapusher_plus.pii_quick_screen = false > ckanext.datapusher_plus.qsv_bin = /usr/local/bin/qsvdp -> ckanext.datapusher_plus.preview_rows = 100 +> ckanext.datapusher_plus.preview_rows = 0 > ckanext.datapusher_plus.download_timeout = 300 > ckanext.datapusher_plus.max_content_length = 1256000000000 > ckanext.datapusher_plus.chunk_size = 16384 @@ -438,7 +436,7 @@ Configure DP+ numerous settings. See [config.py](ckanext/datapusher_plus/config. > ckanext.datapusher_plus.reserved_colnames = _id > ckanext.datapusher_plus.prefer_dmy = false > ckanext.datapusher_plus.ignore_file_hash = true -> ckanext.datapusher_plus.auto_index_threshold = 3 +> ckanext.datapusher_plus.auto_index_threshold = 10 > ckanext.datapusher_plus.auto_index_dates = true > ckanext.datapusher_plus.auto_unique_index = true > ckanext.datapusher_plus.summary_stats_options = @@ -451,11 +449,11 @@ Configure DP+ numerous settings. See [config.py](ckanext/datapusher_plus/config. > ckanext.datapusher_plus.auto_alias_unique = false > ckanext.datapusher_plus.copy_readbuffer_size = 1048576 > ckanext.datapusher_plus.type_mapping = {"String": "text", "Integer": "numeric","Float": "numeric","DateTime": "timestamp","Date": "date","NULL": "text"} -> ckanext.datapusher_plus.auto_spatial_simplication = true -> ckanext.datapusher_plus.spatial_simplication_relative_tolerance = 0.1 +> ckanext.datapusher_plus.auto_spatial_simplification = true +> ckanext.datapusher_plus.spatial_simplification_relative_tolerance = 0.1 > ckanext.datapusher_plus.latitude_fields = latitude,lat -> ckanext.datapusher_plus.longitude_fields = longitude,long,lon -> ckanext.datapusher_plus.jinja2_bytecode_cache_dir = /tmp/jinja2_butecode_cache +> ckanext.datapusher_plus.longitude_fields = longitude,lon +> ckanext.datapusher_plus.jinja2_bytecode_cache_dir = /tmp/jinja2_bytecode_cache > ckanext.datapusher_plus.auto_unzip_one_file = true > ckanext.datapusher_plus.api_token = >``` From fa1276cf7aa20997dde97130dd25ff3c98605d65 Mon Sep 17 00:00:00 2001 From: Joel Natividad <1980690+jqnatividad@users.noreply.github.com> Date: Wed, 20 May 2026 07:24:21 -0400 Subject: [PATCH 4/5] fix: address roborev #2306/#2307/#2308 on PR #326 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit roborev reviewed the three PR #326 commits. Findings actioned: **#2306 (LOW) — test.yml comment.** The "Run unit test suite" step attributed the `--pdbcls` addopt to `setup.cfg`; pytest config moved to `pyproject.toml` (`[tool.pytest.ini_options]`). Comment corrected. **#2307 (LOW) — xlsxb typo in CI workflows.** `ci.yml` and `main.yml` set `formats` with `xlsxb` — a typo for `xlsb` — so an explicitly-configured `.xlsb` resource stayed ungated in CI even though the converter supports it. Corrected `xlsxb` → `xlsb` in all four spots: the `ckanext.datapusher_plus.formats` and legacy `ckan.datapusher.formats` lines in `ci.yml`, the `main.yml` formats line, and `ci.yml`'s test-file `case` branch (whose MIME type, `application/vnd.ms-excel.sheet.binary.macroEnabled.12`, confirms `.xlsb` was the intent). **#2307 (LOW) — missing spatial-key regression test + redundant importorskip.** Added `test_spatial_tolerance_config_key_is_lowercase` (AST drift guard against a revert to the uppercase key typo) and dropped the redundant `pytest.importorskip("ckan")` in `test_formats_covers_every_converter_spreadsheet_extension` (the `formats` fixture already does it). **#2308 (LOW x2) — README config-block notes.** The reworded notes named `config_declaration.yaml` as the sole authoritative source, but `ssl_verify` / `formats` / `unsafe_prefix` / `reserved_colnames` are declared only in `config.py`. Notes now cite both. Also restored the "set only the keys you want to change" guidance and explained why pasting the whole block verbatim is risky (pins every value as an explicit override, defeating future default updates). Not changed (already resolved earlier in the PR stack): * #2307 (MEDIUM) — README documented a misspelled spatial key. The README spelling fix landed in this PR's commit a9e64db; #2307 reviewed the earlier commit 1638bf7 in isolation. Full unit suite: 279 → 280, all passing. Co-Authored-By: Claude Opus 4.7 (1M context) --- .github/workflows/ci.yml | 6 ++-- .github/workflows/main.yml | 2 +- .github/workflows/test.yml | 3 +- README.md | 4 +-- tests/test_formats_config.py | 67 +++++++++++++++++++++++++++++++++--- 5 files changed, 71 insertions(+), 11 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a2f0964e..86a27685 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -375,12 +375,12 @@ jobs: ckanext.datapusher_plus.download_proxy = ckanext.datapusher_plus.ssl_verify = false ckanext.datapusher_plus.upload_log_level = INFO - ckanext.datapusher_plus.formats = csv tsv tab ssv xls xlsx xlsxb xlsm ods geojson shp qgis zip + ckanext.datapusher_plus.formats = csv tsv tab ssv xls xlsx xlsb xlsm ods geojson shp qgis zip # CKAN's bundled test-core.ini sets the legacy `ckan.datapusher.formats` key, which DPP's # _submit_to_datapusher checks FIRST (`tk.config.get("ckan.datapusher.formats") or ...`). # Without overriding it here, the legacy list (csv/xls/xlsx/tsv only) wins and DPP silently # refuses to auto-submit ssv/tab/ods/geojson/shp/qgis/zip resources. - ckan.datapusher.formats = csv tsv tab ssv xls xlsx xlsxb xlsm ods geojson shp qgis zip + ckan.datapusher.formats = csv tsv tab ssv xls xlsx xlsb xlsm ods geojson shp qgis zip ckanext.datapusher_plus.pii_screening = false ckanext.datapusher_plus.pii_found_abort = false ckanext.datapusher_plus.pii_regex_resource_id_or_alias = @@ -717,7 +717,7 @@ jobs: ssv) echo "$name|$file_url|SSV|text/csv|SSV: $filename" >> /tmp/test_files.txt ;; xls) echo "$name|$file_url|XLS|application/vnd.ms-excel|XLS: $filename" >> /tmp/test_files.txt ;; xlsx) echo "$name|$file_url|XLSX|application/vnd.openxmlformats-officedocument.spreadsheetml.sheet|XLSX: $filename" >> /tmp/test_files.txt ;; - xlsxb) echo "$name|$file_url|XLSXB|application/vnd.ms-excel.sheet.binary.macroEnabled.12|XLSXB: $filename" >> /tmp/test_files.txt ;; + xlsb) echo "$name|$file_url|XLSB|application/vnd.ms-excel.sheet.binary.macroEnabled.12|XLSB: $filename" >> /tmp/test_files.txt ;; xlsm) echo "$name|$file_url|XLSM|application/vnd.ms-excel.sheet.macroEnabled.12|XLSM: $filename" >> /tmp/test_files.txt ;; ods) echo "$name|$file_url|ODS|application/vnd.oasis.opendocument.spreadsheet|ODS: $filename" >> /tmp/test_files.txt ;; geojson) echo "$name|$file_url|GEOJSON|application/geo+json|GeoJSON: $filename" >> /tmp/test_files.txt ;; diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 27caa83b..16d1a793 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -265,7 +265,7 @@ jobs: ckanext.datapusher_plus.ssl_verify = false # supports INFO, DEBUG, TRACE - use DEBUG or TRACE when debugging scheming Formulas ckanext.datapusher_plus.upload_log_level = INFO - ckanext.datapusher_plus.formats = csv tsv tab ssv xls xlsx xlsxb xlsm ods geojson shp qgis zip + ckanext.datapusher_plus.formats = csv tsv tab ssv xls xlsx xlsb xlsm ods geojson shp qgis zip ckanext.datapusher_plus.pii_screening = false ckanext.datapusher_plus.pii_found_abort = false ckanext.datapusher_plus.pii_regex_resource_id_or_alias = diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 2e2a19ce..1ea95435 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -78,7 +78,8 @@ jobs: CKAN_INI: /srv/app/src/ckan/test-core.ini run: | set -eu - # -o addopts= overrides setup.cfg's --pdbcls IPython addopt. + # -o addopts= overrides pyproject.toml's + # [tool.pytest.ini_options] --pdbcls IPython addopt. # tests/integration is excluded — it needs the full # docker-compose stack (Prefect, Postgres, Redis, Solr). python3 -m pytest tests/ --ignore=tests/integration \ diff --git a/README.md b/README.md index bf3c8dc1..8413dfa9 100644 --- a/README.md +++ b/README.md @@ -303,7 +303,7 @@ ckan config-tool /etc/ckan/default/ckan.ini "ckanext.datapusher_plus.api_token=$ 7. Add the rest of the DP+ config to your CKAN config (e.g. `/etc/ckan/default/ckan.ini`): -> **Note:** The block below lists DP+'s settings at their current defaults — copy it as a starting point and change only the keys you need. The authoritative defaults are declared in [`config_declaration.yaml`](ckanext/datapusher_plus/config_declaration.yaml). +> **Note:** The block below lists DP+'s settings at their current defaults, for reference. Set only the keys you actually want to change — pasting the whole block pins every value as an explicit override, so a later change to a DP+ default wouldn't reach you. Authoritative defaults live in [`config_declaration.yaml`](ckanext/datapusher_plus/config_declaration.yaml) and [`config.py`](ckanext/datapusher_plus/config.py). ```ini # datapusher-plus settings @@ -411,7 +411,7 @@ scheming.dataset_schemas = ckanext.datapusher_plus:dataset-druf.yaml Configure DP+ numerous settings. See [config.py](ckanext/datapusher_plus/config.py) and [`config_declaration.yaml`](ckanext/datapusher_plus/config_declaration.yaml) for details. -> **Note:** The block below lists DP+'s settings at their current defaults — copy it as a starting point and change only the keys you need. The authoritative defaults are declared in [`config_declaration.yaml`](ckanext/datapusher_plus/config_declaration.yaml). +> **Note:** The block below lists DP+'s settings at their current defaults, for reference. Set only the keys you actually want to change — pasting the whole block pins every value as an explicit override, so a later change to a DP+ default wouldn't reach you. Authoritative defaults live in [`config_declaration.yaml`](ckanext/datapusher_plus/config_declaration.yaml) and [`config.py`](ckanext/datapusher_plus/config.py). >```ini > ckanext.datapusher_plus.download_proxy = diff --git a/tests/test_formats_config.py b/tests/test_formats_config.py index cdcf779f..828094b5 100644 --- a/tests/test_formats_config.py +++ b/tests/test_formats_config.py @@ -1,13 +1,19 @@ # -*- coding: utf-8 -*- """ -Coverage for the ``FORMATS`` config — the gate that decides which -uploaded file formats DataPusher+ will process. +Coverage for two config.py correctness fixes (FORMATS gate + the +spatial-tolerance config key). ``format_converter.py`` can convert ``.xlsm`` and ``.xlsb`` workbooks (both are in ``FormatConverterStage.SPREADSHEET_EXTENSIONS``), but the ``FORMATS`` default in ``config.py`` historically omitted them — so a ``.xlsm`` / ``.xlsb`` resource was silently skipped before it ever -reached the converter. These tests pin: +reached the converter. + +Separately, ``config.py`` read the spatial-simplification relative +tolerance from an UPPERCASE ckan.ini key, inconsistent with every +other DP+ setting. + +These tests pin: 1. ``xlsm`` and ``xlsb`` are in the ``FORMATS`` default — the direct regression guard for that fix. @@ -15,13 +21,20 @@ (``SPREADSHEET_EXTENSIONS``) is present in ``FORMATS`` — a consistency guard so a future "the converter now also handles ``.xyz``" change can't silently leave ``.xyz`` ungated. +3. The spatial-tolerance setting is read from an all-lowercase key, + so a revert to the uppercase typo fails CI. """ from __future__ import annotations +from pathlib import Path + import pytest +REPO_ROOT = Path(__file__).resolve().parent.parent + + @pytest.fixture def formats(): pytest.importorskip("ckan") @@ -40,7 +53,7 @@ def test_formats_covers_every_converter_spreadsheet_extension(formats): # Consistency guard: anything FormatConverterStage advertises in # SPREADSHEET_EXTENSIONS must be gated in by FORMATS, or DP+ would # reject a file format it actually knows how to convert. - pytest.importorskip("ckan") + # (The `formats` fixture already importorskip'd ckan.) from ckanext.datapusher_plus.jobs.stages.format_converter import ( FormatConverterStage, ) @@ -52,3 +65,49 @@ def test_formats_covers_every_converter_spreadsheet_extension(formats): f"not in config.FORMATS — DP+ would skip .{ext.lower()} files " "before the converter ever sees them." ) + + +def test_spatial_tolerance_config_key_is_lowercase(): + """The spatial-simplification relative-tolerance setting must be + read from an all-lowercase ckan.ini key. + + config.py originally read it from + ``ckanext.datapusher_plus.SPATIAL_SIMPLIFICATION_RELATIVE_TOLERANCE`` + — UPPERCASE, unlike every other ~50 DP+ keys — so an operator + setting the documented lowercase form had no effect. AST-parsed + (no CKAN bootstrap) so a revert to the uppercase typo fails CI. + """ + import ast + + 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) + == "SPATIAL_SIMPLIFICATION_RELATIVE_TOLERANCE" + 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.spatial_simplification_relative_tolerance" + ), ( + f"config.py reads the spatial tolerance from {found_key!r}; the " + "key must be all-lowercase to match the rest of DP+'s config " + "namespace and the README." + ) From 7ce2a53a4a0214a1911821a7cf8d26486efd185c Mon Sep 17 00:00:00 2001 From: Joel Natividad <1980690+jqnatividad@users.noreply.github.com> Date: Wed, 20 May 2026 08:10:21 -0400 Subject: [PATCH 5/5] fix: address Copilot review on PR #326 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six Copilot findings, all applied: * **plugin.py had a second, drifting formats list.** ``_submit_to_datapusher`` fell back to its own hardcoded format list when neither ``ckan.datapusher.formats`` nor ``ckanext.datapusher_plus.formats`` was set — and that list lacked ``xlsm``/``xlsb``, so the FORMATS fix would not reach deployments that don't set ``formats`` explicitly. Centralized the fallback on ``conf.FORMATS`` so there is one canonical default that cannot drift. * **Spatial-tolerance config-key rename could break deployments.** The previous commit lowercased the key string; an operator who had set the legacy UPPERCASE key would silently lose the setting. Added a nested fallback to the legacy ``...SPATIAL_SIMPLIFICATION_RELATIVE_TOLERANCE`` key — the same backward-compat shape ``SSL_VERIFY`` already uses. * **tests/test_formats_config.py was runtime/CKAN-dependent.** Rewrote both FORMATS tests to AST-parse the ``config.py`` default list literal and ``format_converter.py``'s ``SPREADSHEET_EXTENSIONS`` — deterministic, CKAN-free config-drift guards in the same style as the other config tests in the repo. * **The spatial-key AST test could capture a nested fallback key.** With the legacy fallback now nested as a second ``tk.config.get`` arg, walking for "the first ``.get``" is fragile. The test now targets the assignment's ``.value`` directly — the outermost ``tk.config.get`` — so it pins the *primary* key regardless of nesting. * **Dead ``use_proxy`` config key in CI workflows.** Removed ``ckanext.datapusher_plus.use_proxy`` from the ci.yml and main.yml CKAN config blocks — there is no such key (DP+ derives proxy use from whether ``download_proxy`` is set). Full unit suite: 280 passing. Co-Authored-By: Claude Opus 4.7 (1M context) --- .github/workflows/ci.yml | 1 - .github/workflows/main.yml | 3 +- ckanext/datapusher_plus/config.py | 10 +- ckanext/datapusher_plus/plugin.py | 22 ++-- tests/test_formats_config.py | 172 +++++++++++++++++++----------- 5 files changed, 125 insertions(+), 83 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 86a27685..3e709479 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -371,7 +371,6 @@ jobs: scheming.dataset_schemas = ckanext.datapusher_plus:dataset-druf.yaml scheming.presets = ckanext.scheming:presets.json scheming.dataset_fallback = false - ckanext.datapusher_plus.use_proxy = false ckanext.datapusher_plus.download_proxy = ckanext.datapusher_plus.ssl_verify = false ckanext.datapusher_plus.upload_log_level = INFO diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 16d1a793..c5ccc369 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -260,8 +260,7 @@ jobs: scheming.dataset_schemas = ckanext.datapusher_plus:dataset-druf.yaml scheming.presets = ckanext.scheming:presets.json scheming.dataset_fallback = false - ckanext.datapusher_plus.use_proxy = false - ckanext.datapusher_plus.download_proxy = + ckanext.datapusher_plus.download_proxy = ckanext.datapusher_plus.ssl_verify = false # supports INFO, DEBUG, TRACE - use DEBUG or TRACE when debugging scheming Formulas ckanext.datapusher_plus.upload_log_level = INFO diff --git a/ckanext/datapusher_plus/config.py b/ckanext/datapusher_plus/config.py index b76c38ef..d7231bd8 100644 --- a/ckanext/datapusher_plus/config.py +++ b/ckanext/datapusher_plus/config.py @@ -253,8 +253,16 @@ AUTO_SPATIAL_SIMPLIFICATION = tk.asbool( tk.config.get("ckanext.datapusher_plus.auto_spatial_simplification", True) ) +# Lowercase key is canonical. The legacy UPPERCASE key +# (``...SPATIAL_SIMPLIFICATION_RELATIVE_TOLERANCE``) is still honoured +# as a fallback so deployments that set it don't silently lose the +# setting — same backward-compat shape as ``SSL_VERIFY`` above. SPATIAL_SIMPLIFICATION_RELATIVE_TOLERANCE = tk.config.get( - "ckanext.datapusher_plus.spatial_simplification_relative_tolerance", "0.1" + "ckanext.datapusher_plus.spatial_simplification_relative_tolerance", + tk.config.get( + "ckanext.datapusher_plus.SPATIAL_SIMPLIFICATION_RELATIVE_TOLERANCE", + "0.1", + ), ) # Latitude and longitude column names diff --git a/ckanext/datapusher_plus/plugin.py b/ckanext/datapusher_plus/plugin.py index 5c8e1667..268c6e5b 100644 --- a/ckanext/datapusher_plus/plugin.py +++ b/ckanext/datapusher_plus/plugin.py @@ -18,6 +18,7 @@ import ckanext.datapusher_plus.logic.action as action import ckanext.datapusher_plus.logic.auth as auth import ckanext.datapusher_plus.cli as cli +import ckanext.datapusher_plus.config as conf tk = p.toolkit @@ -139,22 +140,13 @@ def _submit_to_datapusher(self, resource_dict: dict[str, Any]): ) if not supported_formats: log.debug( - "No supported formats configured,\ - using DataPusher Plus internals" + "No supported formats configured; " + "using the DataPusher+ FORMATS default." ) - supported_formats = [ - "csv", - "xls", - "xlsx", - "tsv", - "ssv", - "tab", - "ods", - "geojson", - "shp", - "qgis", - "zip", - ] + # Reuse the single canonical default from config.py rather + # than a second hardcoded list that would silently drift + # (e.g. miss xlsm/xlsb the way this list once did). + supported_formats = conf.FORMATS submit = ( resource_format diff --git a/tests/test_formats_config.py b/tests/test_formats_config.py index 828094b5..92786b3f 100644 --- a/tests/test_formats_config.py +++ b/tests/test_formats_config.py @@ -13,53 +13,104 @@ tolerance from an UPPERCASE ckan.ini key, inconsistent with every other DP+ setting. -These tests pin: - -1. ``xlsm`` and ``xlsb`` are in the ``FORMATS`` default — the direct - regression guard for that fix. -2. Every spreadsheet extension the converter advertises support for - (``SPREADSHEET_EXTENSIONS``) is present in ``FORMATS`` — a - consistency guard so a future "the converter now also handles +These tests are AST-parsed against the source files — no CKAN import, +no runtime config — so they are deterministic config-drift guards in +the same style as ``test_pii_screening_config_key`` / +``test_issue_142_auto_index_threshold``. They pin: + +1. ``xlsm`` and ``xlsb`` are in the ``FORMATS`` default list literal. +2. Every spreadsheet extension the converter advertises + (``SPREADSHEET_EXTENSIONS``) is present in the ``FORMATS`` default + — a consistency guard so a future "the converter now also handles ``.xyz``" change can't silently leave ``.xyz`` ungated. -3. The spatial-tolerance setting is read from an all-lowercase key, - so a revert to the uppercase typo fails CI. +3. The spatial-tolerance setting's *primary* (outermost) config key + is the all-lowercase name — robust to the legacy-uppercase-key + fallback nested as a second ``tk.config.get`` argument. """ from __future__ import annotations +import ast from pathlib import Path -import pytest - REPO_ROOT = Path(__file__).resolve().parent.parent +CONFIG_PY = REPO_ROOT / "ckanext" / "datapusher_plus" / "config.py" +FORMAT_CONVERTER_PY = ( + REPO_ROOT + / "ckanext" + / "datapusher_plus" + / "jobs" + / "stages" + / "format_converter.py" +) + + +def _find_assignment(source_path, var_name): + """Return the ``ast.Assign`` node that assigns ``var_name`` at any + depth in ``source_path``, or ``None``.""" + tree = ast.parse(source_path.read_text(encoding="utf-8")) + for node in ast.walk(tree): + if isinstance(node, ast.Assign) and any( + getattr(t, "id", None) == var_name for t in node.targets + ): + return node + return None + +def _string_list(list_node): + """Extract the Python string values from an ``ast.List`` of string + constants.""" + assert isinstance(list_node, ast.List), ( + f"expected a list literal, got {type(list_node).__name__}" + ) + return [ + elt.value + for elt in list_node.elts + if isinstance(elt, ast.Constant) and isinstance(elt.value, str) + ] + + +def _formats_default_list(): + """The ``FORMATS`` default list literal from ``config.py``. + + ``FORMATS = tk.config.get("...formats", [])`` — the + default is the call's second positional argument. + """ + assign = _find_assignment(CONFIG_PY, "FORMATS") + assert assign is not None, "FORMATS assignment not found in config.py" + call = assign.value + assert isinstance(call, ast.Call) and len(call.args) >= 2, ( + "expected `FORMATS = tk.config.get(, [])`" + ) + return _string_list(call.args[1]) -@pytest.fixture -def formats(): - pytest.importorskip("ckan") - import ckanext.datapusher_plus.config as conf - return conf.FORMATS +def _converter_spreadsheet_extensions(): + """The ``SPREADSHEET_EXTENSIONS`` list literal from + ``format_converter.py``.""" + assign = _find_assignment(FORMAT_CONVERTER_PY, "SPREADSHEET_EXTENSIONS") + assert assign is not None, ( + "SPREADSHEET_EXTENSIONS not found in format_converter.py" + ) + return _string_list(assign.value) -def test_xlsm_and_xlsb_are_in_formats(formats): - # format_converter can convert both; FORMATS must let them in. +def test_xlsm_and_xlsb_are_in_formats_default(): + # format_converter can convert both; the FORMATS default must let + # them in or DP+ skips the file before the converter sees it. + formats = _formats_default_list() assert "xlsm" in formats assert "xlsb" in formats -def test_formats_covers_every_converter_spreadsheet_extension(formats): +def test_formats_default_covers_every_converter_spreadsheet_extension(): # Consistency guard: anything FormatConverterStage advertises in - # SPREADSHEET_EXTENSIONS must be gated in by FORMATS, or DP+ would - # reject a file format it actually knows how to convert. - # (The `formats` fixture already importorskip'd ckan.) - from ckanext.datapusher_plus.jobs.stages.format_converter import ( - FormatConverterStage, - ) - - formats_lower = {f.lower() for f in formats} - for ext in FormatConverterStage.SPREADSHEET_EXTENSIONS: + # SPREADSHEET_EXTENSIONS must be gated in by the FORMATS default, + # or DP+ would reject a file format it actually knows how to + # convert. + formats_lower = {f.lower() for f in _formats_default_list()} + for ext in _converter_spreadsheet_extensions(): assert ext.lower() in formats_lower, ( f"{ext} is in FormatConverterStage.SPREADSHEET_EXTENSIONS but " f"not in config.FORMATS — DP+ would skip .{ext.lower()} files " @@ -67,47 +118,40 @@ def test_formats_covers_every_converter_spreadsheet_extension(formats): ) -def test_spatial_tolerance_config_key_is_lowercase(): - """The spatial-simplification relative-tolerance setting must be - read from an all-lowercase ckan.ini key. +def test_spatial_tolerance_primary_config_key_is_lowercase(): + """The spatial-simplification relative-tolerance setting's primary + key must be all-lowercase. config.py originally read it from ``ckanext.datapusher_plus.SPATIAL_SIMPLIFICATION_RELATIVE_TOLERANCE`` — UPPERCASE, unlike every other ~50 DP+ keys — so an operator - setting the documented lowercase form had no effect. AST-parsed - (no CKAN bootstrap) so a revert to the uppercase typo fails CI. - """ - import ast + setting the documented lowercase form had no effect. - 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) - == "SPATIAL_SIMPLIFICATION_RELATIVE_TOLERANCE" - 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 == ( + The assertion targets the assignment's ``.value`` directly — the + *outermost* ``tk.config.get`` — so it stays correct even though a + legacy-uppercase-key fallback is nested as that call's second + argument. (Walking for "the first ``.get``" would be fragile here.) + """ + assign = _find_assignment( + CONFIG_PY, "SPATIAL_SIMPLIFICATION_RELATIVE_TOLERANCE" + ) + assert assign is not None, ( + "SPATIAL_SIMPLIFICATION_RELATIVE_TOLERANCE assignment not found " + "in config.py" + ) + primary = assign.value + assert ( + isinstance(primary, ast.Call) + and getattr(primary.func, "attr", None) == "get" + and primary.args + and isinstance(primary.args[0], ast.Constant) + ), "expected the setting to be read via tk.config.get(, ...)" + primary_key = primary.args[0].value + assert primary_key == ( "ckanext.datapusher_plus.spatial_simplification_relative_tolerance" ), ( - f"config.py reads the spatial tolerance from {found_key!r}; the " - "key must be all-lowercase to match the rest of DP+'s config " - "namespace and the README." + f"config.py's primary key for the spatial tolerance is " + f"{primary_key!r}; it must be the all-lowercase name to match " + "the rest of DP+'s config namespace and the README. (A legacy " + "UPPERCASE key may still be honoured as a nested fallback.)" )