fix(portal): resolve cache dataset cache_id to databaseCode - #3316
Open
SantanM wants to merge 3 commits into
Open
fix(portal): resolve cache dataset cache_id to databaseCode#3316SantanM wants to merge 3 commits into
SantanM wants to merge 3 commits into
Conversation
added 3 commits
September 9, 2026 14:07
fc0eb12 gave data marts their own trex catalog, which nothing attaches, so their cache flow run failed on the first task.
Drops resolveCacheWriteTarget; the per-snapshot catalog it returned is never attached in trex.
Re-points data marts created since fc0eb12 at their source row's attached trex catalog.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
FHIR and Strategus snapshots can persist a different catalog from the cache flow’s write target.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Fixes data-mart cache catalog resolution and repairs affected datasets.
Changes:
- Resolves cache dataset IDs to source catalogs.
- Updates cache-flow write targeting and tests.
- Adds a corrective database migration.
File summaries
| File | Description |
|---|---|
dataset.entity.ts |
Adds cache-type resolution rules. |
dataset.entity.test.ts |
Tests the updated type matrix. |
dataset-command.service.ts |
Updates snapshot cache assignment. |
dataset-command.cache-id.test.ts |
Tests snapshot persistence. |
1778417559070-fix-cache-dataset-cache-id.ts |
Repairs existing snapshot rows. |
migration-data-source.ts |
Registers the migration. |
cacheWriteTarget.ts |
Removes obsolete helper. |
cacheWriteTarget.test.ts |
Removes helper tests. |
CachedbController.ts |
Uses the source cache target. |
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+59
to
+60
| if (dataset.type && CACHE_DATASET_TYPES.has(dataset.type)) { | ||
| return dataset.databaseCode ?? null |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Problem
Creating a data mart has been broken since fc0eb12 (#3064). A newly created cache/data-mart
dataset was assigned
cache_id = sanitizeIdForCacheId(snapshotId)— a per-dataset DuckDBcatalog name that nothing in the system ever creates or attaches in trex.
The datamart cache flow hardcodes
use_trex_connection = True, connects to trex pgwire withdbname = options.cache_id, and issues catalog-qualified DDL:(
create_cachedb_file_plugin/copy.py:62-97). Against an unattached catalog that fails:createDatasetSnapshotmakes no trex/attachcall — the onlytrexApiService.attach()callsite in the repo is in
createDataset, for the source dataset — so the snapshot's catalog isnever attached, and the cache file for it is never written.
Root cause
resolveCacheIdspecial-cased onlydialect === 'hana'andtype === 'source'. Every cachedataset type fell through to the sanitized-id branch.
CachedbControllercompounded it bysourcing the cache-file write target from the cache dataset's own
cache_id(
resolveCacheWriteTarget) instead of the source dataset's.Before fc0eb12 the snapshot inherited the source row's
cache_id, whichFixSourceDatasetCacheId1778417559069had set todatabase_code— a real, always-attachedtrex catalog.
AddDatasetCacheId1778417559068had likewise backfilled every pre-existing rowto
database_code, which is why only data marts created after fc0eb12 are affected.Fix
resolveCacheId— added aCACHE_DATASET_TYPESbranch (omop,study,non_omop,hana__omop,hana__non_omop) resolving todatabaseCode. Scoping it to onlyomopwould have left
studyandnon_omopdata marts broken — both are user-selectable(
CopyStudyDialog,AddStudyDialog) and the/snapshotroute triggers the cache flow withno type guard. The
hanaandsourcebranches (Addingsourcetype dataset should set databaseCode as cacheid #2877) are untouched.The fix is in the shared resolver rather than at the call site, so the value persisted to
cache_idand the value handed to trex/attachstill cannot drift.CachedbController— write target back todataset.cacheId ?? databaseCode(the sourcerow). Removed
resolveCacheWriteTargetand its test; it had a single caller.Migration
1778417559070— repairs data marts created since fc0eb12 by inheriting thesource row's
cache_id, matching what the code now computes.source_dataset_idis set onlyby
createDatasetSnapshot, so it targets exactly the affected rows; theIS DISTINCT FROMguard skips rows already correct. Non-reversible by design, same as
…069.webapi,fhirandstrategus_analysisdatasets are deliberately excluded — they genuinelyown a per-dataset catalog. Confirmed on a live stack: each has its own
.dbfile on disk andappears in
duckdb_databases().Verification
Unit tests:
Mechanism, against a live trex:
Migration on a live DB — repaired exactly the one broken row, left
webapiandstrategus_analysisuntouched.End-to-end, with the branch deployed:
POST /gateway/api/dataset/snapshota0150c51-…cache_iddemo_database(pre-fix:_a0150c51_e1ed_42b8_…)COMPLETEDcdma0150c51…+ 2fts_*schemas in thedemo_databasecatalogperson= 2,694 rowsKnown gap, not addressed here
createDatasetSnapshotstill makes no trex/attachcall. That does not matter now that cachedatasets resolve to an always-attached catalog, but it is the reason fc0eb12's per-dataset
catalog design could not work. Anyone reintroducing per-cache-dataset catalogs must add the
attach step and have the flow wait for it.