diff --git a/CHANGELOG.md b/CHANGELOG.md index 169c3681..a1d53a44 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -36,7 +36,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - fix links in metadata statements - fix unhandled promises - fix handling of properties in R4 expansions - +- fix bug validating codes from erroneously coded R4 v2 tables + ### Tx Conformance Statement (paste) diff --git a/library/package-manager.js b/library/package-manager.js index e97a7a62..95fafb0c 100644 --- a/library/package-manager.js +++ b/library/package-manager.js @@ -823,7 +823,7 @@ class PackageContentLoader { try { const content = await fs.readFile(filePath, 'utf8'); - return JSON.parse(content); + return normalizeVersionedCanonicals(JSON.parse(content)); } catch (error) { throw new Error(`Failed to load file ${entry.filename}: ${error.message}`); } @@ -923,4 +923,42 @@ class PackageContentLoader { } -module.exports = { PackageManager, PackageContentLoader }; \ No newline at end of file +/** + * Some R4-era core package resources bake a version into the canonical url itself: + * hl7.fhir.r4.core#4.0.1 publishes the multi-version v2 tables (0006, 0360, 0391) + * as e.g. url "http://terminology.hl7.org/CodeSystem/v2-0360|2.7" with version "0360" + * (the table number!). The base packages can never be republished, so normalize at + * load time: split the pipe into url + version, on the CodeSystems themselves and on + * ValueSet compose include/exclude systems that reference them. Without this, + * provider.system() never matches the system in a coding, so validation fails - + * silently, in the CodeSystem $validate-code case. + */ +function normalizeVersionedCanonicals(resource) { + if (!resource || typeof resource !== 'object') { + return resource; + } + if (resource.resourceType === 'CodeSystem' || resource.resourceType === 'ValueSet') { + if (typeof resource.url === 'string' && resource.url.includes('|')) { + const cut = resource.url.indexOf('|'); + // the url-embedded version wins over the version element (which is "0360" etc) + resource.version = resource.url.substring(cut + 1); + resource.url = resource.url.substring(0, cut); + } + if (resource.resourceType === 'ValueSet' && resource.compose) { + for (const list of [resource.compose.include, resource.compose.exclude]) { + for (const inc of list || []) { + if (typeof inc.system === 'string' && inc.system.includes('|')) { + const cut = inc.system.indexOf('|'); + if (!inc.version) { + inc.version = inc.system.substring(cut + 1); + } + inc.system = inc.system.substring(0, cut); + } + } + } + } + } + return resource; +} + +module.exports = { PackageManager, PackageContentLoader, normalizeVersionedCanonicals }; \ No newline at end of file diff --git a/packages/packages.js b/packages/packages.js index 40052002..366451d0 100644 --- a/packages/packages.js +++ b/packages/packages.js @@ -605,10 +605,6 @@ class PackagesModule { this.crawler = new PackageCrawler(this.config, this.db, this.stats); } - // The feed uses the versioned package.tgz url as the (permalink) GUID, so reusing - // the link as the GUID keeps a later crawl from inserting a duplicate. - const guid = link; - const buffer = await this.crawler.fetchUrl(link); const npm = await this.crawler.extractNpmPackage(buffer, link); @@ -618,22 +614,37 @@ class PackagesModule { } const idver = npm.id + '#' + npm.version; - const replaced = await this.deleteVersionsByGuid(guid); + + // Delete whatever is stored for this package version, matching on the id#version + // actually found in the fetched tarball. This used to match on GUID = link, but the + // stored GUID is whatever the feed said at crawl time (http vs https, or another + // permalink form), so an exact string match could silently delete nothing and leave + // the stale copy in place alongside the newly stored one. + const old = await this.deleteVersionsByIdVersion(npm.id, npm.version); + + // Keep the old row's GUID when there was one - it is the feed's permalink, and + // reusing it stops the next crawl from re-inserting the package as a duplicate. + // When we never had the package, the link is the best available GUID (the feeds + // use the versioned package.tgz url as the permalink GUID anyway). + const guid = old.guids[0] || link; const itemLog = { status: '??' }; await this.crawler.store(link, link, guid, new Date(), buffer, idver, itemLog); - pckLog.info('Force-updated ' + idver + ' from ' + link + ' (replaced ' + replaced + ' existing row(s))'); - return { status: 'updated', id: npm.id, version: npm.version, replaced }; + pckLog.info('Force-updated ' + idver + ' from ' + link + ' (replaced ' + old.count + ' existing row(s), guid ' + guid + ')'); + return { status: 'updated', id: npm.id, version: npm.version, replaced: old.count }; } - // Delete a stored package version and all of its child rows, by GUID. - deleteVersionsByGuid(guid) { + // Delete a stored package version and all of its child rows, by package id and + // version. Resolves to { count, guids }: how many PackageVersions rows went, and + // the distinct GUIDs they were stored under (so a caller can reuse the permalink). + deleteVersionsByIdVersion(id, version) { return new Promise((resolve, reject) => { - this.db.all('SELECT PackageVersionKey FROM PackageVersions WHERE GUID = ?', [guid], (err, rows) => { + this.db.all('SELECT PackageVersionKey, GUID FROM PackageVersions WHERE Id = ? AND Version = ?', [id, version], (err, rows) => { if (err) return reject(err); const keys = (rows || []).map(r => r.PackageVersionKey); - if (keys.length === 0) return resolve(0); + const guids = [...new Set((rows || []).map(r => r.GUID))]; + if (keys.length === 0) return resolve({ count: 0, guids: [] }); const ph = keys.map(() => '?').join(','); const stmts = [ 'DELETE FROM PackageFHIRVersions WHERE PackageVersionKey IN (' + ph + ')', @@ -642,7 +653,7 @@ class PackagesModule { 'DELETE FROM PackageVersions WHERE PackageVersionKey IN (' + ph + ')' ]; const runNext = (i) => { - if (i >= stmts.length) return resolve(keys.length); + if (i >= stmts.length) return resolve({ count: keys.length, guids }); this.db.run(stmts[i], keys, (e) => e ? reject(e) : runNext(i + 1)); }; runNext(0); @@ -1289,8 +1300,10 @@ class PackagesModule { // // POST /update-package { "links": ["http://hl7.org/fhir/uv/ips/2.0.1/package.tgz", ...] } // - // The link is the versioned package.tgz url, which is exactly the GUID the feed uses, - // so the replacement keeps the same GUID and a later crawl won't create a duplicate. + // The link is the versioned package.tgz url. The stored copy to replace is found by + // the id#version inside the fetched tarball (not by matching the link against the + // stored GUID, which can differ in scheme or form from what the feed published), and + // the replacement keeps the old row's GUID so a later crawl won't create a duplicate. // If config.updateToken is set, the request must carry it in the x-update-token header. this.router.post('/update-package', async (req, res) => { const start = Date.now(); diff --git a/tests/tx/provider-tho-precedence.test.js b/tests/tx/provider-tho-precedence.test.js index af0395ff..81bba07c 100644 --- a/tests/tx/provider-tho-precedence.test.js +++ b/tests/tx/provider-tho-precedence.test.js @@ -9,7 +9,11 @@ const { Provider } = require('../../tx/provider'); * version precedence therefore left the stale core copy as the unversioned * default, so $lookup with no version couldn't find codes (like 'payconc') * that only exist in the THO copy. A THO resource must displace same-URL - * resources from core packages, regardless of version. + * resources from core packages as the unversioned default, regardless of + * version — but explicitly versioned entries survive, because the core + * packages carry historical versions (the R4 v2 tables 0006/0360/0391 at + * 2.1/2.3.1/2.4/2.6/2.7, normalized from their pipe-versioned urls at load + * time) that THO does not publish at all. */ const SELFPAY = 'http://terminology.hl7.org/CodeSystem/coverage-selfpay'; @@ -41,8 +45,11 @@ describe('Provider.addCodeSystem — THO vs core package precedence', () => { // the unversioned default must be the THO copy despite its lower version expect(p.codeSystems.get(SELFPAY).sourcePackage).toBe('hl7.terminology.r4#6.0.2'); expect(p.codeSystems.get(SELFPAY).version).toBe('1.0.1'); - // the core copy is dropped entirely (matches the Java drop()) - expect(p.codeSystems.has(SELFPAY + '|4.0.1')).toBe(false); + // the core copy loses only the unversioned default slot; it stays addressable + // by explicit version, because core packages carry historical versions (e.g. the + // normalized R4 v2 tables 0006/0360/0391) that THO does not publish at all + expect(p.codeSystems.has(SELFPAY + '|4.0.1')).toBe(true); + expect(p.codeSystems.get(SELFPAY + '|4.0.1').sourcePackage).toBe('hl7.fhir.r4.core#4.0.1'); expect(p.codeSystems.get(SELFPAY + '|1.0.1').sourcePackage).toBe('hl7.terminology.r4#6.0.2'); }); @@ -52,7 +59,16 @@ describe('Provider.addCodeSystem — THO vs core package precedence', () => { cs(SELFPAY, '4.0.1', 'hl7.fhir.r4.core#4.0.1') ); expect(p.codeSystems.get(SELFPAY).sourcePackage).toBe('hl7.terminology.r4#6.0.2'); - expect(p.codeSystems.has(SELFPAY + '|4.0.1')).toBe(false); + // versioned addressability is kept in this load order too + expect(p.codeSystems.get(SELFPAY + '|4.0.1').sourcePackage).toBe('hl7.fhir.r4.core#4.0.1'); + }); + + test('a core resource yields its versioned slot to THO only when THO provides that exact version', () => { + const p = newProvider( + cs(SELFPAY, '1.0.1', 'hl7.terminology.r4#6.0.2'), + cs(SELFPAY, '1.0.1', 'hl7.fhir.r4.core#4.0.1') + ); + expect(p.codeSystems.get(SELFPAY + '|1.0.1').sourcePackage).toBe('hl7.terminology.r4#6.0.2'); }); test('THO does not displace same-url resources from non-core packages', () => { diff --git a/tx/provider.js b/tx/provider.js index 31ae32a1..a19433b1 100644 --- a/tx/provider.js +++ b/tx/provider.js @@ -498,7 +498,11 @@ class Provider { // resource that came from a FHIR core package, regardless of version. if (Provider.#isTHOPackage(cs.sourcePackage)) { for (const [key, t] of [...this.codeSystems]) { - if (t.url === cs.url && VersionUtilities.isCorePackage(t.sourcePackage)) { + // only the bare-url (default) entry is displaced; explicitly versioned + // entries (key = url|version) stay addressable, because the core packages + // carry historical versions (e.g. the normalized R4 v2 tables 0006/0360/0391 + // at 2.1/2.3.1/2.4/2.6/2.7) that hl7.terminology does not publish at all + if (key === t.url && t.url === cs.url && VersionUtilities.isCorePackage(t.sourcePackage)) { this.codeSystems.delete(key); } } @@ -510,8 +514,14 @@ class Provider { if (!existing || (!yieldsToTHO && cs.isMoreRecent(existing))) { this.codeSystems.set(cs.url, cs); } - if (cs.version && !yieldsToTHO) { - this.codeSystems.set(cs.vurl, cs); + if (cs.version) { + // a versioned entry only yields to THO when THO itself provides that exact version + const vExisting = this.codeSystems.get(cs.vurl); + const vYieldsToTHO = vExisting && Provider.#isTHOPackage(vExisting.sourcePackage) + && VersionUtilities.isCorePackage(cs.sourcePackage); + if (!vYieldsToTHO) { + this.codeSystems.set(cs.vurl, cs); + } } } diff --git a/tx/vs/vs-package.js b/tx/vs/vs-package.js index e3eb52c0..9bb7cd0e 100644 --- a/tx/vs/vs-package.js +++ b/tx/vs/vs-package.js @@ -1,4 +1,5 @@ const path = require('path'); +const fs = require('fs').promises; const { AbstractValueSetProvider } = require('./vs-api'); const { PackageContentLoader } = require('../../library/package-manager'); const { ValueSetDatabase } = require('./vs-database'); @@ -51,6 +52,20 @@ class PackageValueSetProvider extends AbstractValueSetProvider { } this.valueSetMap = await this.database.loadAllValueSets(this.sourcePackage()); + + // Self-heal caches built before pipe-versioned canonical urls were normalized at + // load time (the R4 v2 tables 0006/0360/0391 - see normalizeVersionedCanonicals in + // package-manager.js): if any cached value set still carries a "system|version" + // include or a pipe in its url, rebuild this cache from the package. + if (dbExists && this.#hasPipeVersionedCanonicals()) { + await this.database.close(); + await fs.rm(this.dbPath, { force: true }); + this.database = new ValueSetDatabase(this.dbPath); + await this.database.create(); + await this._populateDatabase(); + this.valueSetMap = await this.database.loadAllValueSets(this.sourcePackage()); + } + // Mark each value set as cached as it enters the store: it is retained in // memory for the process lifetime, so its compose.include.filter elements // persist and a provider may memoise resolved filter analysis on them @@ -65,6 +80,32 @@ class PackageValueSetProvider extends AbstractValueSetProvider { this.initialized = true; } + /** + * True if any loaded value set still has a pipe-versioned canonical (url or + * compose include/exclude system) - the signature of a cache written before + * load-time normalization existed. + */ + #hasPipeVersionedCanonicals() { + for (const vs of this.valueSetMap.values()) { + if (!vs) { + continue; + } + if (typeof vs.url === 'string' && vs.url.includes('|')) { + return true; + } + if (vs.compose) { + for (const list of [vs.compose.include, vs.compose.exclude]) { + for (const inc of list || []) { + if (typeof inc.system === 'string' && inc.system.includes('|')) { + return true; + } + } + } + } + } + return false; + } + async close() { await this.database.close(); } diff --git a/tx/workers/validate.js b/tx/workers/validate.js index 659c87df..086321d7 100644 --- a/tx/workers/validate.js +++ b/tx/workers/validate.js @@ -753,6 +753,7 @@ class ValueSetChecker { messages.push(msg); } } else { + this.worker.opContext.addNote(this.valueSet, 'Include for ' + this.worker.renderer.displayCoded(cc.system, cc.version) + ' not considered for "' + this.worker.renderer.displayCoded(system, version) + '"', this.indentCount); result = false; } for (let u of cc.valueSet || []) { @@ -875,6 +876,7 @@ class ValueSetChecker { } } } else { + this.worker.opContext.addNote(this.valueSet, 'ValueSet has neither compose nor expansion - cannot determine membership', this.indentCount); result = false; } } @@ -1286,7 +1288,10 @@ class ValueSetChecker { } i++; } - if (ok === false && !this.valueSet.jsonObj.internallyDefined) { + // for an internally defined value set (CodeSystem validation), the code-system-level + // message is preferred - but if nothing produced an error, this is the only + // explanation there will be, so it can't be suppressed + if (ok === false && (!this.valueSet.jsonObj.internallyDefined || !op.hasErrors())) { let mid, m, p; if (mode === 'codeableConcept') { mid = 'TX_GENERAL_CC_ERROR_MESSAGE';