fix bug validating codes from erroneously coded R4 v2 table

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)

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 };
+/**
+ * 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 };

packages/packages.js

@@ -605,10 +605,6 @@
       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 @@
     }
 
     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 @@
           '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);
@@ -954,7 +965,7 @@
       pckLog.info('Starting initial package crawler...');
 
       // Run crawler in background (non-blocking)
       setImmediate(async () => {
         try {
           await this.runCrawler();
           pckLog.info('Initial package crawler completed successfully');
@@ -1289,8 +1300,10 @@
     //
     //   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();

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', () => {

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);
+      }
     }
   }
 

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();
   }

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';