Skip to content

fix(#310): preserve file_hash on metadata stage's resource re-fetch#312

Merged
jqnatividad merged 2 commits into
mainfrom
fix-310-metadata-hash-drop
May 18, 2026
Merged

fix(#310): preserve file_hash on metadata stage's resource re-fetch#312
jqnatividad merged 2 commits into
mainfrom
fix-310-metadata-hash-drop

Conversation

@jqnatividad

@jqnatividad jqnatividad commented May 18, 2026

Copy link
Copy Markdown
Collaborator

Summary

Closes #310.

MetadataStage._update_resource_metadata re-fetches the resource from CKAN (via dsu.get_resource) to pick up datastore_create's side effects before layering preview / preview_rows on top — added by d653129c two days ago. That re-fetch wholesale replaces context.resource with the on-disk CKAN copy, which still has the empty initial hash value at that point in the flow. The hash the DownloadStage computed and set on context.resource["hash"] (line 129 of download.py) is silently discarded, so the update_resource call that follows persists an empty hash.

The regression has been live for ~2 days but was masked because MD5 hashes also looked empty when omitted — no one was reading the value, only checking for existence. PR #309's BLAKE3-default post-merge smoke test caught it via the "CKAN persists a 64-hex digest" check.

Fix

One-liner immediately after the re-fetch — restore context.resource["hash"] from the in-flight context.file_hash when present.

Edge case explicitly handled: if context.file_hash is empty (corner case — upstream bug, or some not-currently-observed code path), we leave whatever hash the re-fetch returned alone rather than clobbering a real stored hash with an empty string.

Test plan

  • 209/209 Python unit tests pass on dpp-test (was 206; +3).
  • New tests/test_metadata_hash_persistence.py regression suite:
    • Hash from context.file_hash lands on the persisted resource (the main bug).
    • Empty context.file_hash does NOT clobber a re-fetched hash (safety).
    • Current run's hash overrides a stale stored hash (the real production path).
  • Integration test in CIintegration-test job on the head commit (0550b3b) passed against the full Prefect + Postgres + Solr + Redis + DP+-worker stack.
  • Manual smoke (post-merge) — re-ran the feat(#221): configurable file-hash algorithm (blake3 default, sha256, md5) #309 smoke against the merged fix. Uploaded a 7-row CSV through the integration stack; CKAN's hash field is now populated end-to-end (9d024321107231ce794e377f48d9953583b603bc0e82d8328119cc56ba65cb68, matching b3sum on the host). Resources uploaded to the same dataset BEFORE the merge still show empty hash — proving the fix specifically. See follow-up comment below.

🤖 Generated with Claude Code

Closes #310.

``MetadataStage._update_resource_metadata`` re-fetches the resource
from CKAN (via ``dsu.get_resource``) to pick up ``datastore_create``'s
side effects before layering preview/preview_rows on top — added by
``d653129c`` two days ago. That re-fetch wholesale replaces
``context.resource`` with the on-disk CKAN copy, which still has the
empty initial ``hash`` value at that point in the flow. The hash the
DownloadStage computed and set on ``context.resource["hash"]`` (line
129 of ``download.py``) is silently discarded, so the
``update_resource`` call that follows persists an empty hash.

The regression has been live for ~2 days but was masked because MD5
hashes also looked empty when omitted — no one was reading the value,
only checking for existence. PR #309's BLAKE3-default post-merge
smoke test caught it via the "CKAN persists a 64-hex digest" check.

Fix is a one-liner immediately after the re-fetch: restore
``context.resource["hash"]`` from the in-flight ``context.file_hash``
when present. The DownloadStage skip path (``IGNORE_FILE_HASH`` or
``_should_skip_upload`` matched) leaves ``file_hash`` set, so this
path also covers the dedup case correctly.

Edge case explicitly handled: if ``context.file_hash`` is empty (a
corner case — upstream bug, or some not-currently-observed code
path), we leave whatever hash the re-fetch returned alone rather
than clobbering a real stored hash with an empty string.

Tests in new ``test_metadata_hash_persistence.py``:

* Hash from ``context.file_hash`` lands on the persisted resource.
* Empty ``context.file_hash`` does NOT clobber a re-fetched hash.
* Current run's hash overrides a stale stored hash.

209/209 unit tests pass on dpp-test (was 206; +3).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes a regression where MetadataStage._update_resource_metadata re-fetches the CKAN resource and unintentionally discards the in-flight hash computed by DownloadStage, resulting in an empty persisted resource hash (issue #310).

Changes:

  • Restore context.resource["hash"] from context.file_hash immediately after the CKAN re-fetch (when context.file_hash is non-empty).
  • Add regression tests covering hash persistence, non-clobbering behavior when file_hash is empty, and overriding stale stored hashes.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
ckanext/datapusher_plus/jobs/stages/metadata.py Restores the computed file hash after the resource re-fetch so update_resource persists the correct digest.
tests/test_metadata_hash_persistence.py Adds unit-level regression coverage for hash persistence across the metadata-stage re-fetch.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tests/test_metadata_hash_persistence.py Outdated
…sitive

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@jqnatividad jqnatividad merged commit 4a65f9e into main May 18, 2026
1 check passed
@jqnatividad

Copy link
Copy Markdown
Collaborator Author

Post-merge smoke — verifying the fix end-to-end against the integration stack.

Setup

  • Pulled merged main into the integration stack's mounted DP+ checkout.
  • Restarted the Prefect worker container to drop stale bytecode (the metadata.py change is the only relevant module).
  • Re-injected ckanext.datapusher_plus.api_token into tests/integration/ckan.ini (lost on git checkout main — bind-mounted file is the source of truth, no api_token line in tracked content) and restarted CKAN.

End-to-end test

  • Uploaded a 7-row CSV (smoke-post-fix.csv) via resource_create.
  • Polled resource_show until datastore_active == True.
Field Value Length
Expected b3sum on host 9d024321107231ce794e377f48d9953583b603bc0e82d8328119cc56ba65cb68 64 hex
CKAN resource hash 9d024321107231ce794e377f48d9953583b603bc0e82d8328119cc56ba65cb68 64 hex

Match.

Specificity check

The same dataset still has older resources from BEFORE the #312 fix landed:

391eeb73-aa58-4eb7-adb0-18448aa8a613  smoke-post-fix.csv (pre-fix)  hash=''
94c597c8-e881-4c6a-a756-a629b5a15fcd  smoke-post-fix.csv (pre-fix)  hash=''
0abc153e-a3be-48cd-b0a3-b9bc5ebb2b62  smoke-post-fix.csv (post-fix) hash='9d02...'

Empty hash on pre-fix uploads, populated hash on post-fix upload — same code path, same input. The fix is doing exactly what it claims to do.

Carry-over: still relevant

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

metadata stage drops file hash on re-fetch — persisted resource hash is always empty

2 participants