diff --git a/library/keep-alive.js b/library/keep-alive.js new file mode 100644 index 00000000..06eb65af --- /dev/null +++ b/library/keep-alive.js @@ -0,0 +1,18 @@ +// Keep-alive timeouts for the HTTP server behind a reverse proxy. +// +// nginx keeps idle upstream connections open for 60 s (keepalive_timeout in an upstream block), +// while Node closes an idle one after 5 s. When nginx reuses a connection at the moment Node +// closes it, the request fails with a reset, and nginx does not retry a POST: the client gets a +// 502. Node must therefore keep idle connections open longer than the proxy does, and +// headersTimeout must be longer still, or Node applies it to the idle connection first. + +const KEEP_ALIVE_TIMEOUT_MS = 65000; +const HEADERS_TIMEOUT_MS = 66000; + +function applyKeepAliveTimeouts(server) { + server.keepAliveTimeout = KEEP_ALIVE_TIMEOUT_MS; + server.headersTimeout = HEADERS_TIMEOUT_MS; + return server; +} + +module.exports = { applyKeepAliveTimeouts, KEEP_ALIVE_TIMEOUT_MS, HEADERS_TIMEOUT_MS }; diff --git a/server.js b/server.js index cc724cd9..442d7e12 100644 --- a/server.js +++ b/server.js @@ -18,6 +18,7 @@ const escape = require('escape-html'); const { resolveWithin } = require('./library/path-safety'); const { readCgroupMemoryLimit } = require('./library/cgroup-memory'); const memoryGuard = require('./library/memory-guard'); +const { applyKeepAliveTimeouts } = require('./library/keep-alive'); const { NpmAudit } = require('./library/npm-audit'); const Mailer = require('./library/mailer'); const DailyReport = require('./library/daily-report'); @@ -836,10 +837,12 @@ async function startServer() { }); // Start server - app.listen(PORT, () => { + const server = app.listen(PORT, () => { stats.markStarted(packageJson.version); serverLog.info(`=== Server running on http://localhost:${PORT} ===`); }); + // idle connections stay open longer than nginx keeps them (see library/keep-alive.js) + applyKeepAliveTimeouts(server); npmAudit.start(); dailyReport = new DailyReport(config.dailyReport, { log: Logger.getInstance().child({ module: 'daily-report' }), diff --git a/tests/cs/cs-country.test.js b/tests/cs/cs-country.test.js index f16b0866..36fe1a90 100644 --- a/tests/cs/cs-country.test.js +++ b/tests/cs/cs-country.test.js @@ -261,6 +261,23 @@ describe('CountryCodeServices', () => { expect(results[0].code).toBe('US'); }); + test('a pattern with alternation matches whole codes only', async () => { + // '^' + 'US|CA' + '$' would be "starts with US or ends with CA", taking in USA + const ctxt = await provider.getPrepContext(false); + await provider.filter(ctxt, true, 'code', 'regex', 'US|CA'); + const filters = await provider.executeFilters(ctxt); + const filter = filters[0]; + + const results = []; + filter.cursor = -1; + while (await provider.filterMore(ctxt, filter)) { + const concept = await provider.filterConcept(ctxt, filter); + results.push(concept.code); + } + + expect(results.sort()).toEqual(['CA', 'US']); + }); + test('should filter all 2-letter codes', async () => { const ctxt = await provider.getPrepContext(false); await provider.filter(ctxt, true, 'code', 'regex', '[A-Z]{2}'); diff --git a/tests/cs/cs-loinc.test.js b/tests/cs/cs-loinc.test.js index 64038168..4f774e7d 100644 --- a/tests/cs/cs-loinc.test.js +++ b/tests/cs/cs-loinc.test.js @@ -7,6 +7,7 @@ const { OperationContext } = require('../../tx/operation-context'); const {validateParameter} = require("../../library/utilities"); const {Designations, SearchFilterText} = require("../../tx/library/designations"); const {TestUtilities} = require("../test-utilities"); +const {CodeSystem} = require('../../tx/library/codesystem'); const folders = require('../../library/folder-setup'); describe('LOINC Module Import', () => { @@ -916,6 +917,33 @@ describe('LOINC Provider', () => { expect(size).toBeGreaterThanOrEqual(0); }); + // LOINC's own text search needs every word (no display in the test data has "serum + // glucose"); a supplement's designations must match the same way, not on any one word + test('a supplement\'s designations match a text filter only when they have every word', async () => { + const supplement = new CodeSystem({ + resourceType: 'CodeSystem', url: 'http://example.org/loinc-supplement', version: '1', status: 'active', + content: 'supplement', supplements: 'http://loinc.org', language: 'en', + concept: [ + { code: '101448-9', designation: [{ language: 'en', value: 'Serum antibody screen' }] }, + { code: '104597-0', display: 'Blood glucose by meter' }, + { code: '102820-8', designation: [{ language: 'en', value: 'Glucose level in serum' }] } + ] + }); + const supplemented = await factory.build(opContext, [supplement]); + try { + const filterContext = await supplemented.getPrepContext(true); + await supplemented.searchFilter(filterContext, new SearchFilterText('serum glucose'), false); + const filters = await supplemented.executeFilters(filterContext); + const codes = []; + while (await supplemented.filterMore(filterContext, filters[0])) { + codes.push(await supplemented.code(await supplemented.filterConcept(filterContext, filters[0]))); + } + expect(codes).toEqual(['102820-8']); + } finally { + supplemented.close(); + } + }); + test('lists text filter matches in description order, sorted or not', async () => { for (const sort of [false, true]) { const filterContext = await provider.getPrepContext(true); @@ -1097,6 +1125,38 @@ describe('LOINC Provider', () => { ]); }); + // The importer stores class type 3 as 'Claims attachment' (1,161 codes in LOINC 2.83, such + // as 18719-5). The test data has none, so one code is given it here for the test + test('CLASSTYPE 3 (Claims attachment): $lookup reports it, and the CLASSTYPE = 3 filter finds it', async () => { + const code = '104597-0'; + const rw = new sqlite3.Database(testDbPath); + const run = (sql, args = []) => new Promise((resolve, reject) => rw.run(sql, args, function (err) { err ? reject(err) : resolve(this); })); + const get = (sql, args = []) => new Promise((resolve, reject) => rw.get(sql, args, (err, row) => err ? reject(err) : resolve(row))); + const prop = await get(`SELECT Properties.PropertyKey, Properties.PropertyValueKey FROM Properties, PropertyTypes, Codes + WHERE Properties.PropertyTypeKey = PropertyTypes.PropertyTypeKey AND PropertyTypes.Description = 'CLASSTYPE' + AND Properties.CodeKey = Codes.CodeKey AND Codes.Code = ?`, [code]); + const added = await run(`INSERT INTO PropertyValues (Value) VALUES ('Claims attachment')`); + try { + await run('UPDATE Properties SET PropertyValueKey = ? WHERE PropertyKey = ?', [added.lastID, prop.PropertyKey]); + + const params = []; + await provider.extendLookup(code, ['CLASSTYPE'], params); + const classType = params.find(p => p.name === 'property' && p.part.find(x => x.name === 'code').valueCode === 'CLASSTYPE'); + expect(classType.part.find(x => x.name === 'value').valueString).toBe('3'); + expect(classType.part.find(x => x.name === 'description').valueString).toBe('Claims attachment'); + + const filterContext = await provider.getPrepContext(true); + await provider.filter(filterContext, true, 'CLASSTYPE', '=', '3'); + const filters = await provider.executeFilters(filterContext); + expect(await provider.filterSize(filterContext, filters[0])).toBe(1); + expect((await provider.filterLocate(filterContext, filters[0], code)).code).toBe(code); + } finally { + await run('UPDATE Properties SET PropertyValueKey = ? WHERE PropertyKey = ?', [prop.PropertyValueKey, prop.PropertyKey]); + await run('DELETE FROM PropertyValues WHERE PropertyValueKey = ?', [added.lastID]); + await new Promise(resolve => rw.close(resolve)); + } + }); + test('should extend lookup with properties and relationships', async () => { const testCode = expectedResults.basic.knownCodes[0]; const paramSet = []; diff --git a/tests/cs/cs-snomed.test.js b/tests/cs/cs-snomed.test.js index 45e9b11a..c2457ad7 100644 --- a/tests/cs/cs-snomed.test.js +++ b/tests/cs/cs-snomed.test.js @@ -1526,6 +1526,31 @@ describe('SNOMED CT descendent-leaf and is-not-a filters', () => { expect(isA.length + notA.length).toBe(provider.sct.concepts.count()); expect(notA.filter(c => isA.includes(c))).toEqual([]); }); + + // 41000147108 is from the Dutch edition, which the test edition doesn't include. As an + // unknown code does in other code systems, it selects nothing (is-not-a: everything) in an + // expansion. (doesFilter still says no to it, so $validate-code reports the filter as not + // understood where it asks.) + test('a concept the edition does not have selects no concepts, rather than failing', async () => { + const unknown = '41000147108'; + expect(provider.sct.concepts.findConcept(provider.sct.stringToIdOrZero(unknown)).found).toBe(false); + const iterated = async (op, value) => { + const fc = await provider.getPrepContext(true); + await provider.filter(fc, true, 'concept', op, value); + const [set] = await provider.executeFilters(fc); + const out = []; + while (await provider.filterMore(fc, set)) { + out.push(await provider.code(await provider.filterConcept(fc, set))); + } + return out; + }; + for (const op of ['=', 'is-a', 'descendent-of', 'child-of', 'generalizes', 'descendent-leaf', 'in']) { + expect(await iterated(op, unknown)).toEqual([]); + expect(await located(op, unknown, '16763008')).toBe(false); + } + expect(await iterated('in', unknown + ',16763008')).toEqual(['16763008']); + expect((await iterated('is-not-a', unknown)).length).toBe(provider.sct.concepts.count()); + }); }); // display regex (S6): a concept matches when it has an active description (FSN or synonym, diff --git a/tests/library/keep-alive.test.js b/tests/library/keep-alive.test.js new file mode 100644 index 00000000..41a53f38 --- /dev/null +++ b/tests/library/keep-alive.test.js @@ -0,0 +1,40 @@ +// Idle connections must outlive the reverse proxy's (nginx: 60 s), or nginx reuses a connection +// Node is closing and a POST on it fails with a 502. Node's default is 5 s. + +const http = require('http'); +const net = require('net'); +const { applyKeepAliveTimeouts, KEEP_ALIVE_TIMEOUT_MS, HEADERS_TIMEOUT_MS } = require('../../library/keep-alive'); + +describe('keep-alive timeouts', () => { + test('idle connections stay open longer than nginx keeps them, and headersTimeout is longer still', () => { + const server = applyKeepAliveTimeouts(http.createServer()); + expect(server.keepAliveTimeout).toBe(KEEP_ALIVE_TIMEOUT_MS); + expect(server.keepAliveTimeout).toBeGreaterThan(60000); + expect(server.headersTimeout).toBe(HEADERS_TIMEOUT_MS); + expect(server.headersTimeout).toBeGreaterThan(server.keepAliveTimeout); + }); + + test('a connection idle for longer than Node\'s 5 s default can still be reused', async () => { + const server = applyKeepAliveTimeouts(http.createServer((req, res) => res.end('ok'))); + await new Promise(resolve => server.listen(0, '127.0.0.1', resolve)); + const socket = net.connect(server.address().port, '127.0.0.1'); + let closed = false; + socket.on('close', () => { closed = true; }); + const send = () => new Promise((resolve, reject) => { + socket.once('data', d => resolve(d.toString())); + socket.once('error', reject); + socket.once('close', () => reject(new Error('connection closed'))); + socket.write('GET / HTTP/1.1\r\nHost: x\r\nConnection: keep-alive\r\n\r\n'); + }); + try { + expect(await send()).toMatch(/^HTTP\/1.1 200/); + await new Promise(resolve => setTimeout(resolve, 6000)); + expect(closed).toBe(false); + expect(await send()).toMatch(/^HTTP\/1.1 200/); + } finally { + socket.destroy(); + server.closeAllConnections(); + await new Promise(resolve => server.close(resolve)); + } + }, 15000); +}); diff --git a/tests/tx/batch-validate-yield.test.js b/tests/tx/batch-validate-yield.test.js new file mode 100644 index 00000000..621396f5 --- /dev/null +++ b/tests/tx/batch-validate-yield.test.js @@ -0,0 +1,42 @@ +/** + * $batch-validate-code gives the event loop a turn between entries. + * + * Validating one entry never yields by itself, so a 1,000-entry batch from an IG build used to + * hold the event loop for its whole duration (~1 s on production hardware), and every other + * request arriving meanwhile waited for it (a single lookup typically waited 0.4-0.9 s). + * processBatch now calls checkAndYield before each entry; checkAndYield only actually yields + * after YIELD_INTERVAL ms of compute, so small batches are unaffected. + */ +const request = require('supertest'); +const { getTestApp, shutdownTestApp } = require('./setup'); +const { OperationContext } = require('../../tx/operation-context'); + +const CS_URL = 'http://example.org/fhir/CodeSystem/batch-yield-cs'; +const CS = { + resourceType: 'CodeSystem', url: CS_URL, version: '1.0.0', status: 'active', content: 'complete', + concept: [{ code: 'a', display: 'Alpha' }, { code: 'b', display: 'Beta' }] +}; +const entry = (code) => ({ name: 'validation', resource: { resourceType: 'Parameters', parameter: [{ name: 'code', valueCode: code }] } }); + +describe('$batch-validate-code yields between entries', () => { + let app; + beforeAll(async () => { app = await getTestApp(); }, 60000); + afterAll(async () => { await shutdownTestApp(); }); + + test('checkAndYield is called once per entry and the results are unchanged', async () => { + const spy = jest.spyOn(OperationContext.prototype, 'checkAndYield'); + try { + const codes = ['a', 'zz', 'b', 'a', 'b']; + const res = await request(app).post('/tx/r5/CodeSystem/$batch-validate-code') + .set('Content-Type', 'application/json') + .send({ resourceType: 'Parameters', parameter: [{ name: 'tx-resource', resource: CS }, { name: 'url', valueUri: CS_URL }, ...codes.map(entry)] }); + expect(res.status).toBe(200); + const calls = spy.mock.calls.filter((c) => c[0] === 'batch-validate').length; + expect(calls).toBe(codes.length); + const results = res.body.parameter.map((r) => r.resource.parameter.find((p) => p.name === 'result').valueBoolean); + expect(results).toEqual([true, false, true, true, true]); + } finally { + spy.mockRestore(); + } + }); +}); diff --git a/tests/tx/cache-id.test.js b/tests/tx/cache-id.test.js index a7ca4f0b..d70fc130 100644 --- a/tests/tx/cache-id.test.js +++ b/tests/tx/cache-id.test.js @@ -200,6 +200,17 @@ describe('cache-id protocol (integration)', () => { expect(isUnknownCacheError(res)).toBe(true); }); + test('CodeSystem/$validate-code with an unknown cache-id is answered (it used to never respond)', async () => { + const res = await request(app).post('/tx/r5/CodeSystem/$validate-code').timeout(10000) + .set('Content-Type', 'application/json') + .send({ resourceType: 'Parameters', parameter: [ + { name: 'cache-id', valueId: 'never-issued-this-cache' }, + { name: 'coding', valueCoding: { system: 'http://hl7.org/fhir/administrative-gender', code: 'male' } } + ] }); + expect(res.status).toBe(404); + expect(isUnknownCacheError(res)).toBe(true); + }); + test('by url with no cache-id and a VS not in the library fails (not a silent pass)', async () => { const r = await post(app, VALIDATE, [ { name: 'url', valueString: colorsVS.url }, diff --git a/tests/tx/codesystem-map.test.js b/tests/tx/codesystem-map.test.js new file mode 100644 index 00000000..03d88ab3 --- /dev/null +++ b/tests/tx/codesystem-map.test.js @@ -0,0 +1,42 @@ +/** + * CodeSystemMap keeps an index of supplements by the URL they supplement, maintained on every + * set/delete, so Provider.loadSupplements() does not scan every loaded CodeSystem per lookup. + */ +const { CodeSystemMap } = require('../../tx/provider'); + +const cs = (url, version, extra = {}) => ({ url, version, vurl: url + '|' + version, jsonObj: { url, version, content: 'complete', ...extra } }); +const supp = (url, version, of) => cs(url, version, { content: 'supplement', supplements: of }); + +describe('CodeSystemMap supplement index', () => { + test('indexes supplements by the supplemented URL, without its version', () => { + const m = new CodeSystemMap(); + m.set('http://x/base', cs('http://x/base', '1')); + m.set('http://x/s1', supp('http://x/s1', '1', 'http://x/base')); + m.set('http://x/s2|2', supp('http://x/s2', '2', 'http://x/base|1')); + m.set('http://x/other', supp('http://x/other', '1', 'http://y/base')); + expect([...m.supplementsOf('http://x/base')].map((c) => c.url).sort()).toEqual(['http://x/s1', 'http://x/s2']); + expect([...m.supplementsOf('http://y/base')].map((c) => c.url)).toEqual(['http://x/other']); + expect([...m.supplementsOf('http://z')]).toEqual([]); + }); + + test('replacing or deleting an entry keeps the index right', () => { + const m = new CodeSystemMap(); + m.set('k', supp('http://x/s1', '1', 'http://x/base')); + m.set('k', supp('http://x/s1', '2', 'http://x/other')); + expect([...m.supplementsOf('http://x/base')]).toEqual([]); + expect([...m.supplementsOf('http://x/other')].map((c) => c.version)).toEqual(['2']); + m.delete('k'); + expect([...m.supplementsOf('http://x/other')]).toEqual([]); + expect(m.supplementIndex.size).toBe(0); + m.set('a', supp('http://x/s1', '1', 'http://x/base')); + m.clear(); + expect([...m.supplementsOf('http://x/base')]).toEqual([]); + }); + + test('behaves as a Map', () => { + const m = new CodeSystemMap([['a', cs('http://x/a', '1')]]); + expect(m.get('a').url).toBe('http://x/a'); + expect(m.size).toBe(1); + expect(m instanceof Map).toBe(true); + }); +}); diff --git a/tests/tx/designations.test.js b/tests/tx/designations.test.js index b89ec59f..9d23dbb1 100644 --- a/tests/tx/designations.test.js +++ b/tests/tx/designations.test.js @@ -615,6 +615,16 @@ describe('Designations', () => { expect(filter.passes('cardiac rhythm')).toBe(false); }); + test('passesAll needs every word of the filter, passes only one', () => { + const filter = new SearchFilterText('serum glucose'); + + expect(filter.passes('Cholesterol in Serum')).toBe(true); + expect(filter.passesAll('Cholesterol in Serum')).toBe(false); + expect(filter.passesAll('Glucose [Mass/volume] in Serum or Plasma')).toBe(true); + expect(filter.passesAll('Glucoses, serum')).toBe(true); // stemmed, in any order + expect(new SearchFilterText('').passesAll('anything')).toBe(true); + }); + test('should calculate match scores', () => { const filter = new SearchFilterText('blood pressure'); const stems = ['blood', 'pressure', 'measurement']; diff --git a/tests/tx/expand.test.js b/tests/tx/expand.test.js index b04ca664..7ceae798 100644 --- a/tests/tx/expand.test.js +++ b/tests/tx/expand.test.js @@ -477,6 +477,69 @@ describe('Expand Worker', () => { }); }); + describe('Value sets that cannot be expanded', () => { + const expand = (parameter) => request(app) + .post('/tx/r5/ValueSet/$expand') + .set('Accept', 'application/json') + .set('Content-Type', 'application/json') + .send({ resourceType: 'Parameters', parameter }); + const messages = (response) => response.body.issue.map(i => (i.details && i.details.text) || i.diagnostics); + + // The extension changes what the filter means, so refusing is right; but the problem is in + // the request's content, not the server + test('a filter with an unknown modifier extension: 422', async () => { + const response = await expand([{ + name: 'valueSet', + resource: { + resourceType: 'ValueSet', + url: 'http://example.org/fhir/ValueSet/unknown-filter-modifier', + status: 'active', + compose: { + include: [{ + system: 'http://hl7.org/fhir/administrative-gender', + filter: [{ + modifierExtension: [{ url: 'http://example.org/fhir/StructureDefinition/unknown-op-modifier', valueString: 'childOf' }], + property: 'concept', op: 'is-a', value: 'male' + }] + }] + } + } + }]); + + expect(response.status).toBe(422); + expect(response.body.issue[0].code).toBe('business-rule'); + expect(messages(response)[0]).toContain('http://example.org/fhir/StructureDefinition/unknown-op-modifier'); + }); + + // A value set in the request needn't have a url: the message names it by its id, or as + // '??', which is what tx-ecosystem's snomed-expand-inactive2 expects - never 'undefined' + test('too costly: a value set with no url is named by its id, or as \'??\'', async () => { + const valueSet = { resourceType: 'ValueSet', status: 'active', compose: { include: [{ system: 'http://hl7.org/fhir/administrative-gender' }] } }; + for (const [vs, name] of [[valueSet, '??'], [{ ...valueSet, id: 'genders' }, 'genders'], + [{ ...valueSet, url: 'http://example.org/fhir/ValueSet/genders' }, 'http://example.org/fhir/ValueSet/genders']]) { + for (const extra of [[], [{ name: 'activeOnly', valueBoolean: true }]]) { + const response = await expand([{ name: 'valueSet', resource: vs }, { name: 'limit', valueInteger: 2 }, ...extra]); + expect(response.status).toBe(422); + expect(response.body.issue[0].code).toBe('too-costly'); + expect(messages(response)[0]).toBe(`The value set '${name}' expansion has too many codes to produce (>2)`); + } + } + }); + + // As for a code system the server doesn't have (422 not-found): there is nothing to expand from + test('codes from a code system with no content (not-present), or from a supplement: 422', async () => { + for (const [system, code, text] of [['http://hl7.org/fhir/color-rgb', '#FF0000', 'has no content'], + ['http://hl7.org/fhir/CodeSystem/example-supplement', 'chol', 'defines a supplement']]) { + const response = await expand([{ + name: 'valueSet', + resource: { resourceType: 'ValueSet', status: 'active', compose: { include: [{ system, concept: [{ code }] }] } } + }]); + expect(response.status).toBe(422); + expect(messages(response)[0]).toContain(text); + } + }); + }); + describe('Expansion caching', () => { test('should return same expansion for identical requests', async () => { // Make the same expand request twice diff --git a/tests/tx/library-partial-version.test.js b/tests/tx/library-partial-version.test.js new file mode 100644 index 00000000..ce9845ae --- /dev/null +++ b/tests/tx/library-partial-version.test.js @@ -0,0 +1,38 @@ +'use strict'; + +// A partial version (a SNOMED CT edition URI without its date, or a semver major.minor) resolves +// to the latest version loaded for it, whatever the library's load order. + +const { Library } = require('../../tx/library'); + +const SCT = 'http://snomed.info/sct'; +const US = 'http://snomed.info/sct/731000124108'; + +function factory(system, version, partial) { + return { system: () => system, version: () => version, getPartialVersion: () => partial }; +} + +function load(order) { + const lib = new Library(null, null, { info() {}, debug() {}, warn() {}, error() {} }); + order.forEach((f, i) => lib.registerProvider('test', f, i === 0)); + return lib; +} + +describe('library: partial-version keys', () => { + const older = factory(SCT, US + '/version/20240301', US); + const newer = factory(SCT, US + '/version/20250901', US); + + test.each([['newest first', [newer, older]], ['oldest first', [older, newer]]])('SNOMED edition-only pin, %s', (_, order) => { + const lib = load(order); + expect(lib.codeSystemFactories.get(SCT + '|' + US)).toBe(newer); + // exact versions are untouched + expect(lib.codeSystemFactories.get(SCT + '|' + US + '/version/20240301')).toBe(older); + expect(lib.codeSystemFactories.get(SCT + '|' + US + '/version/20250901')).toBe(newer); + }); + + test.each([['newest first', ['1.2.10', '1.2.3']], ['oldest first', ['1.2.3', '1.2.10']]])('semver major.minor, %s', (_, vs) => { + const fs = vs.map((v) => factory('http://example.org/cs', v, '1.2')); + const lib = load(fs); + expect(lib.codeSystemFactories.get('http://example.org/cs|1.2').version()).toBe('1.2.10'); + }); +}); diff --git a/tests/tx/loinc-import-answer-lists.test.js b/tests/tx/loinc-import-answer-lists.test.js new file mode 100644 index 00000000..33f974b2 --- /dev/null +++ b/tests/tx/loinc-import-answer-lists.test.js @@ -0,0 +1,46 @@ +// LOINC importer: an externally defined answer list (no AnswerStringId) adds the list, and no +// blank Answer code. AnswerList.csv in 2.83 has 65 such rows; they all became one Answer whose +// code is '', so LOINC had one code too many (257,267 rather than 257,266) +const { LoincDataMigrator } = require('../../tx/importers/import-loinc.module'); + +function migrator() { + const m = new LoincDataMigrator(); + m.codeKey = 0; + m.codes = new Map(); + m.rels = new Map([['Answer', 40], ['AnswerList', 39]]); + m.relationships = []; + m.addRelationship = (db, s, t, r) => m.relationships.push([s, t, r]); + m.addExternalMapping = () => {}; + return m; +} + +function db() { + const inserts = []; + return { inserts, run: (sql, params) => inserts.push(params) }; +} + +// AnswerListId, AnswerListName, AnswerListOID, ExtDefinedYN, ExtDefinedAnswerListCodeSystem, +// ExtDefinedAnswerListLink, AnswerStringId, LocalAnswerCode, LocalAnswerCodeSystem, SequenceNumber, DisplayText, ... +const row = (list, name, answer, display) => [list, name, '', answer ? 'N' : 'Y', '', '', answer, '', '', '1', display, '', '', '', '', '', '', '', '']; + +describe('LOINC import: answer lists', () => { + test('an externally defined list (no AnswerStringId) is a code, with no blank answer', async () => { + const m = migrator(); + const d = db(); + await m.processListItem(d, row('LL1892-0', 'ICD-9_ICD-10', '', ''), null); + await m.processListItem(d, row('LL1894-6', 'RxNorm', '', ''), 'LL1892-0'); + expect(d.inserts.map(p => p[1])).toEqual(['LL1892-0', 'LL1894-6']); + expect(m.codes.has('')).toBe(false); + expect(m.relationships).toEqual([]); + }); + + test('a list with answers still adds each answer once, linked both ways', async () => { + const m = migrator(); + const d = db(); + await m.processListItem(d, row('LL1-1', 'Yes/No', 'LA33-6', 'Yes'), null); + await m.processListItem(d, row('LL1-1', 'Yes/No', 'LA32-8', 'No'), 'LL1-1'); + await m.processListItem(d, row('LL2-2', 'Yes', 'LA33-6', 'Yes'), 'LL1-1'); + expect(d.inserts.map(p => [p[1], p[2]])).toEqual([['LL1-1', 3], ['LA33-6', 4], ['LA32-8', 4], ['LL2-2', 3]]); + expect(m.relationships).toHaveLength(6); + }); +}); diff --git a/tests/tx/usage-tracker-yield.test.js b/tests/tx/usage-tracker-yield.test.js new file mode 100644 index 00000000..6717d83f --- /dev/null +++ b/tests/tx/usage-tracker-yield.test.js @@ -0,0 +1,37 @@ +// The ConceptUsageTracker's startup scan reads every value set, about a second of work on a full +// source list; it used to run without a break, so requests arriving just after startup waited +// for all of it. It now gives the event loop a turn every few milliseconds. + +const ConceptUsageTracker = require('../../tx/usage-tracker'); + +function busy(ms) { + const end = Date.now() + ms; + while (Date.now() < end) { /* compute */ } +} + +function libraryOf(count) { + const vs = (i) => ({ jsonObj: { compose: { include: [{ system: 'http://example.org/cs', concept: [{ code: 'c' + (i % 7) }] }] } } }); + return { + valueSetProviders: [{ + listAllValueSets: async () => Array.from({ length: count }, (_, i) => 'http://example.org/vs/' + i), + fetchValueSet: async (url) => { busy(1); return vs(Number(url.split('/').pop())); } + }] + }; +} + +describe('ConceptUsageTracker scan', () => { + test('other work runs during the scan, and the counts are unchanged', async () => { + const tracker = new ConceptUsageTracker(); + let ranDuringScan = 0; + let scanning = true; + const tick = () => { if (scanning) { ranDuringScan++; setImmediate(tick); } }; + setImmediate(tick); + const count = await tracker.scanValueSets(libraryOf(100)); + scanning = false; + expect(count).toBe(100); + expect(ranDuringScan).toBeGreaterThan(3); + const usages = tracker.usages('http://example.org/cs'); + expect(usages.get('c0').count).toBe(15); + expect(usages.get('c6').count).toBe(14); + }); +}); diff --git a/tests/tx/validate-enumerated-miss.test.js b/tests/tx/validate-enumerated-miss.test.js new file mode 100644 index 00000000..e52c9681 --- /dev/null +++ b/tests/tx/validate-enumerated-miss.test.js @@ -0,0 +1,64 @@ +/** + * A code that is not in an enumerated include costs one lookup of the code, not one per listed code. + * + * Before checking whether a listed concept is the same concept written in another form (ICD-11's + * short code and entity id), the checker located the code once per listed concept, though the + * answer (does the code have another form at all?) does not depend on the listed concept. A miss + * against a 33,000-code VSAC list never finished. It is now asked once per include. + */ +const request = require('supertest'); +const { getTestApp, shutdownTestApp } = require('./setup'); +const { FhirCodeSystemProvider } = require('../../tx/cs/cs-cs'); + +const CS_URL = 'http://example.org/fhir/CodeSystem/enumerated-miss-cs'; +const VS_URL = 'http://example.org/fhir/ValueSet/enumerated-miss-vs'; +const N = 300; +const codes = Array.from({ length: N }, (_, i) => 'c' + i); +const CS = { + resourceType: 'CodeSystem', url: CS_URL, version: '1.0.0', status: 'active', content: 'complete', + concept: [...codes, 'unlisted'].map(code => ({ code, display: 'Display ' + code })) +}; +const VS = { + resourceType: 'ValueSet', url: VS_URL, version: '1.0.0', status: 'active', + compose: { include: [{ system: CS_URL, concept: codes.map(code => ({ code })) }] } +}; + +async function validate(app, code) { + return request(app).post('/tx/r5/ValueSet/$validate-code') + .set('Content-Type', 'application/json') + .send({ resourceType: 'Parameters', parameter: [ + { name: 'tx-resource', resource: CS }, + { name: 'valueSet', resource: VS }, + { name: 'coding', valueCoding: { system: CS_URL, code } } + ] }); +} + +const resultOf = (res) => res.body.parameter.find(p => p.name === 'result').valueBoolean; + +describe('$validate-code: a miss against an enumerated include', () => { + let app; + beforeAll(async () => { app = await getTestApp(); }, 60000); + afterAll(async () => { await shutdownTestApp(); }); + + test.each([ + ['in the code system but not listed', 'unlisted'], + ['not in the code system', 'no-such-code'] + ])('a code %s is located a handful of times, not once per listed code', async (_label, code) => { + const spy = jest.spyOn(FhirCodeSystemProvider.prototype, 'locate'); + try { + const res = await validate(app, code); + expect(res.status).toBe(200); + expect(resultOf(res)).toBe(false); + const lookupsOfCode = spy.mock.calls.filter(c => c[0] === code).length; + expect(lookupsOfCode).toBeLessThan(10); + } finally { + spy.mockRestore(); + } + }); + + test('a listed code still validates', async () => { + const res = await validate(app, 'c150'); + expect(res.status).toBe(200); + expect(resultOf(res)).toBe(true); + }); +}); diff --git a/tests/vs/vs-vsac-refresh-interval.test.js b/tests/vs/vs-vsac-refresh-interval.test.js new file mode 100644 index 00000000..650dbf79 --- /dev/null +++ b/tests/vs/vs-vsac-refresh-interval.test.js @@ -0,0 +1,49 @@ +// The VSAC refresh timer: Node fires a timer whose delay is above 2^31-1 ms (about 596 hours) after +// 1 ms instead, so a long refreshIntervalHours used to run the refresh back to back + +const { VSACValueSetProvider } = require('../../tx/vs/vs-vsac'); + +function provider(refreshIntervalHours) { + const p = Object.create(VSACValueSetProvider.prototype); + p.refreshIntervalHours = refreshIntervalHours; + p.refreshTimer = null; + return p; +} + +describe('VSAC refresh interval', () => { + let warn; + beforeEach(() => { + warn = jest.spyOn(console, 'warn').mockImplementation(() => {}); + }); + afterEach(() => warn.mockRestore()); + + test('an ordinary interval is used as given', () => { + expect(provider(24)._refreshIntervalMs()).toBe(24 * 3600 * 1000); + expect(provider(0.5)._refreshIntervalMs()).toBe(1800 * 1000); + expect(warn).not.toHaveBeenCalled(); + }); + + test('an interval longer than a timer can wait is clamped, not turned into 1 ms', () => { + expect(provider(100000)._refreshIntervalMs()).toBe(2 ** 31 - 1); + expect(warn).toHaveBeenCalledTimes(1); + }); + + test('a value that is not a positive number falls back to 24 hours', () => { + for (const h of [-1, 'abc', NaN, Infinity]) { + expect(provider(h)._refreshIntervalMs()).toBe(24 * 3600 * 1000); + } + }); + + test('the timer is started with the clamped delay', () => { + const spy = jest.spyOn(global, 'setInterval'); + const p = provider(100000); + try { + p._startRefreshTimer(); + expect(spy).toHaveBeenCalledTimes(1); + expect(spy.mock.calls[0][1]).toBe(2 ** 31 - 1); + } finally { + clearInterval(p.refreshTimer); + spy.mockRestore(); + } + }); +}); diff --git a/tx/cs/cs-api.js b/tx/cs/cs-api.js index 911b1ff4..998a6e8a 100644 --- a/tx/cs/cs-api.js +++ b/tx/cs/cs-api.js @@ -342,7 +342,8 @@ class CodeSystemProvider { /** * The codes whose displays or designations in a supplement in use, in the search language, - * pass a text filter - a supplement's designations are searched like the code system's own + * pass a text filter - a supplement's designations are searched like the code system's own, + * so every word of the filter must match (as LOINC's and SNOMED CT's own text search need) * * @param {SearchFilterText} filter * @param {Language} searchLang - from searchLanguage() @@ -353,12 +354,12 @@ class CodeSystemProvider { for (const supplement of this.supplements || []) { const suppLang = supplement.jsonObj.language || this.defLang(); for (const concept of supplement.getAllConcepts()) { - if (concept.display && CodeSystemProvider.inSearchLanguage(suppLang, searchLang) && filter.passes(concept.display)) { + if (concept.display && CodeSystemProvider.inSearchLanguage(suppLang, searchLang) && filter.passesAll(concept.display)) { codes.add(concept.code); continue; } for (const d of concept.designation || []) { - if (d.value && CodeSystemProvider.inSearchLanguage(d.language, searchLang, suppLang) && filter.passes(d.value)) { + if (d.value && CodeSystemProvider.inSearchLanguage(d.language, searchLang, suppLang) && filter.passesAll(d.value)) { codes.add(concept.code); break; } diff --git a/tx/cs/cs-loinc.js b/tx/cs/cs-loinc.js index 33a3a664..25da1f82 100644 --- a/tx/cs/cs-loinc.js +++ b/tx/cs/cs-loinc.js @@ -53,13 +53,16 @@ function mapRelationship(equivalence, fromLoinc) { } } +// CLASSTYPE as the importer stores it (descClassType in import-loinc.module.js) and its number. +// The importer writes 'Claims attachment', singular; the plural is accepted too const classTypes = { '1': 'Laboratory class', '2': 'Clinical class', - '3': 'Claims attachments', + '3': 'Claims attachment', '4': 'Surveys', 'Laboratory class' : '1', 'Clinical class' : '2', + 'Claims attachment' : '3', 'Claims attachments' : '3', 'Surveys' : '4' }; diff --git a/tx/cs/cs-snomed.js b/tx/cs/cs-snomed.js index de742e15..d3a78bdb 100644 --- a/tx/cs/cs-snomed.js +++ b/tx/cs/cs-snomed.js @@ -946,7 +946,7 @@ class SnomedServices { const conceptResult = this.concepts.findConcept(id); if (!conceptResult.found) { - throw new Error(`The SNOMED CT Concept ${id} is not known`); + continue; // a concept the edition doesn't have adds nothing } const refSetIndex = this.getConceptRefSet(conceptResult.index, false); @@ -3555,6 +3555,16 @@ class SnomedProvider extends BaseCSServices { if (id === 0n && op !== 'in') { throw new Error(`Invalid concept ID: ${value}`); } + // A concept the edition doesn't have (one from another edition, say) selects no + // concepts - every concept, for is-not-a - as an unknown code does in other code systems + if (op !== 'in' && !this.sct.concepts.findConcept(id).found) { + const none = new SnomedFilterContext(); + if (op === 'is-not-a') { + none.descendants = this.sct._eclEnumerateAllConcepts(this.opContext); + } + filterContext.filters.push(none); + return null; + } switch (op) { case '=': { diff --git a/tx/importers/import-loinc.module.js b/tx/importers/import-loinc.module.js index 68090ac9..fa5e3b18 100644 --- a/tx/importers/import-loinc.module.js +++ b/tx/importers/import-loinc.module.js @@ -1118,6 +1118,12 @@ class LoincDataMigrator { } const answerCode = removeQuotes(items[6]); + if (!answerCode) { + // an externally defined answer list (ExtDefinedYN = Y: ICD-10, RxNorm, ...) has one row with + // no AnswerStringId. The list is a code; the row names no answer, so there is nothing to add + // (it used to become a single Answer code '' shared by all 65 such lists) + return; + } let answerCodeKey; if (this.codes.has(answerCode)) { diff --git a/tx/library.js b/tx/library.js index 5098a813..b0df16cc 100644 --- a/tx/library.js +++ b/tx/library.js @@ -31,7 +31,7 @@ const {IETFLanguageCodeFactory} = require("./cs/cs-lang"); const {LanguageDefinitions} = require("../library/languages"); const {VersionUtilities} = require("../library/version-utilities"); const {ListCodeSystemProvider} = require("./cs/cs-provider-list"); -const { Provider } = require("./provider"); +const { Provider, CodeSystemMap } = require("./provider"); const {I18nSupport} = require("../library/i18nsupport"); const folders = require('../library/folder-setup'); const {VSACValueSetProvider} = require("./vs/vs-vsac"); @@ -119,7 +119,12 @@ class Library { this.codeSystemFactories.set(factory.system()+"|"+ver, factory); const verMin = factory.getPartialVersion(); if (verMin) { - this.codeSystemFactories.set(factory.system()+"|"+verMin, factory); + // a partial version (major.minor, or a SNOMED CT edition URI without its date) names the + // latest version loaded for it, whatever the order the library lists them in + const prev = this.codeSystemFactories.get(factory.system()+"|"+verMin); + if (!prev || VersionUtilities.compareVersionsGeneral(ver, prev.version() ?? "") > 0) { + this.codeSystemFactories.set(factory.system()+"|"+verMin, factory); + } } } @@ -1172,7 +1177,7 @@ class Library { const provider = new Provider(); provider.i18n = this.i18n; provider.codeSystemFactories = new Map(this.codeSystemFactories); // all of them - provider.codeSystems = new Map(); + provider.codeSystems = new CodeSystemMap(); provider.valueSetProviders = []; provider.conceptMapProviders = []; provider.path = path; diff --git a/tx/library/designations.js b/tx/library/designations.js index bbac6bbf..a5fb1c7b 100644 --- a/tx/library/designations.js +++ b/tx/library/designations.js @@ -82,6 +82,33 @@ class SearchFilterText { return returnRating ? {passes: false, rating: 0} : false; } + /** + * Whether every word of the filter is in the value, a word matching as in passes() (its + * stem starts with the filter word's stem). passes() needs only one word + */ + passesAll(value) { + validateParameter(value, 'value', String); + + if (this.isNull) { + return true; + } + + const stems = []; + let i = 0; + while (i < value.length) { + if (this._isAlphaNumeric(value[i])) { + const j = i; + while (i < value.length && this._isAlphaNumeric(value[i])) { + i++; + } + stems.push(this.stemmer.stem(value.substring(j, i).toLowerCase())); + } else { + i++; + } + } + return this.stems.every(filterStem => stems.some(stem => stem.startsWith(filterStem))); + } + /** * Check if designations pass the filter */ diff --git a/tx/library/extensions.js b/tx/library/extensions.js index 2e52c9b3..43680835 100644 --- a/tx/library/extensions.js +++ b/tx/library/extensions.js @@ -36,6 +36,8 @@ const Extensions = { if (element.jsonObj) { element = element.jsonObj } + // A modifier extension this server doesn't know changes what the element means, so it + // can't be processed: a problem with the resource's content (422), not with the server if (element.modifierExtension) { let urls = new Set(); for (const extension of element.modifierExtension) { @@ -44,9 +46,9 @@ const Extensions = { const resId = resource ? resource : ""; const urlList = [...urls].join('\', \''); if (urls.size > 1) { - throw new Issue("error", "business-rule", null, null, 'Cannot process resource '+resId+' at "' + name + '" due to the presence of modifier extensions '+urlList); + throw new Issue("error", "business-rule", null, null, 'Cannot process resource '+resId+' at "' + name + '" due to the presence of modifier extensions '+urlList, null, 422); } else { - throw new Issue("error", "business-rule", null, null, 'Cannot process resource '+resId+' at "' + name + '" due to the presence of the modifier extension '+urlList); + throw new Issue("error", "business-rule", null, null, 'Cannot process resource '+resId+' at "' + name + '" due to the presence of the modifier extension '+urlList, null, 422); } } return true; diff --git a/tx/operation-context.js b/tx/operation-context.js index 29837db6..2b729750 100644 --- a/tx/operation-context.js +++ b/tx/operation-context.js @@ -824,7 +824,7 @@ const CHECK_FREQUENCY = 100; // I/O. checkAndYield() awaits a real setImmediate every YIELD_INTERVAL ms of // compute, so pending I/O (new connections, /metadata, timers) runs between // slices and one heavy $expand can no longer time out every other client. -const YIELD_INTERVAL = 25; +const YIELD_INTERVAL = 10; const { setImmediate: yieldToEventLoop } = require('timers/promises'); class OperationContext { diff --git a/tx/provider.js b/tx/provider.js index d5ebef2e..c7e17303 100644 --- a/tx/provider.js +++ b/tx/provider.js @@ -24,6 +24,64 @@ const { applyIdPrefix, CORE_PREFIX } = require("./library/resource-ids"); * - a list of concept map providers that can provide concept map instances * */ +/** + * The provider's CodeSystem map, with an index of the supplements it holds by the canonical URL + * they supplement. The index is maintained on every set/delete, so loadSupplements() looks at the + * few candidates instead of scanning every loaded CodeSystem on every code-system lookup (that scan + * was about a fifth of the event loop's busy time under IG-build traffic). + */ +class CodeSystemMap extends Map { + constructor(entries) { + super(); + this.supplementIndex = new Map(); + if (entries) { + for (const [k, v] of entries) this.set(k, v); + } + } + + static supplementedUrl(cs) { + const u = cs && cs.jsonObj && cs.jsonObj.content === 'supplement' ? cs.jsonObj.supplements : null; + return u ? String(u).split('|')[0] : null; + } + + set(k, v) { + if (super.has(k)) this.#unindex(k, super.get(k)); + super.set(k, v); + const u = CodeSystemMap.supplementedUrl(v); + if (u) { + let m = this.supplementIndex.get(u); + if (!m) this.supplementIndex.set(u, m = new Map()); + m.set(k, v); + } + return this; + } + + delete(k) { + if (super.has(k)) this.#unindex(k, super.get(k)); + return super.delete(k); + } + + clear() { + this.supplementIndex.clear(); + super.clear(); + } + + #unindex(k, v) { + const u = CodeSystemMap.supplementedUrl(v); + const m = u && this.supplementIndex.get(u); + if (m) { + m.delete(k); + if (!m.size) this.supplementIndex.delete(u); + } + } + + /** the CodeSystems held here that supplement url (any version) */ + supplementsOf(url) { + const m = this.supplementIndex.get(url); + return m ? m.values() : []; + } +} + class Provider { i18n; fhirVersion; @@ -116,7 +174,8 @@ class Provider { for (let csp of this.codeSystemFactories.values()) { csp.listSupplements(supplements, url, version, statedSupplements); } - for (let cs of this.codeSystems.values()) { + const candidates = this.codeSystems instanceof CodeSystemMap ? this.codeSystems.supplementsOf(url) : this.codeSystems.values(); + for (let cs of candidates) { if (cs.isSupplementFor(url, version)) { if (cs.isGlobalSupplement() || this.isStatedSupplement(cs, statedSupplements)) { supplements.set(cs.vurl, cs); @@ -749,4 +808,4 @@ class Provider { } -module.exports = { Provider }; +module.exports = { CodeSystemMap, Provider }; diff --git a/tx/tx.js b/tx/tx.js index 09d5ec42..f5d5630f 100644 --- a/tx/tx.js +++ b/tx/tx.js @@ -465,7 +465,8 @@ class TXModule { res.setHeader('Content-Type', jsonFmt); this.checkProperJson(jsonStr); responseSize = Buffer.byteLength(jsonStr, 'utf8'); - result = originalJson(data); + // send the string built above: res.json(data) would serialise the resource again + result = res.send(jsonStr); } // Log the request with request ID diff --git a/tx/usage-tracker.js b/tx/usage-tracker.js index 4fb0c7f3..39229c4e 100644 --- a/tx/usage-tracker.js +++ b/tx/usage-tracker.js @@ -1,4 +1,10 @@ +const { setImmediate: yieldToEventLoop } = require('timers/promises'); + +// The startup scan reads every value set (thousands with the npm packages, VSAC), about a second +// of work: give requests a turn every YIELD_INTERVAL_MS of it. +const YIELD_INTERVAL_MS = 10; + class ConceptUsageTracker { constructor() { @@ -7,9 +13,14 @@ class ConceptUsageTracker { async scanValueSets(library) { let c = 0; + let sliceStart = Date.now(); for (let vsp of library.valueSetProviders) { let list = await vsp.listAllValueSets(); for (let url of list) { + if (Date.now() - sliceStart >= YIELD_INTERVAL_MS) { + await yieldToEventLoop(); + sliceStart = Date.now(); + } let vs = await vsp.fetchValueSet(url); if (vs && vs.jsonObj.compose) { if (await this.scanValueSet(vs.jsonObj.compose)) { diff --git a/tx/vs/vs-vsac.js b/tx/vs/vs-vsac.js index 337a3573..cea52c95 100644 --- a/tx/vs/vs-vsac.js +++ b/tx/vs/vs-vsac.js @@ -11,6 +11,9 @@ const { applyIdPrefix, stripIdPrefix } = require("../library/resource-ids"); // Persisted watermark for the phase-1b _lastUpdated scan. const VSAC_LAST_UPDATED_KEY = 'vsac_last_updated_date'; +// The longest delay a Node timer accepts; a longer one fires after 1 ms. +const MAX_TIMER_MS = 2 ** 31 - 1; + // Canonical URL prefix for VSAC value sets, so operators can enter a bare OID. const VSAC_VALUESET_URL_PREFIX = 'http://cts.nlm.nih.gov/fhir/ValueSet/'; @@ -115,7 +118,7 @@ class VSACValueSetProvider extends AbstractValueSetProvider { clearInterval(this.refreshTimer); } - const intervalMs = this.refreshIntervalHours * 60 * 60 * 1000; + const intervalMs = this._refreshIntervalMs(); this.refreshTimer = setInterval(async () => { try { await this.refreshValueSets(); @@ -126,6 +129,26 @@ class VSACValueSetProvider extends AbstractValueSetProvider { }, intervalMs); } + /** + * The refresh interval in ms. Node runs a timer whose delay is above 2^31-1 ms (about 596 hours) + * or not a positive number after 1 ms instead, so the refresh would run back to back: such a + * setting is clamped (or, when it isn't a positive number, replaced by the 24 hour default). + * @private + */ + _refreshIntervalMs() { + const hours = Number(this.refreshIntervalHours); + if (!Number.isFinite(hours) || hours <= 0) { + console.warn(`VSAC: refreshIntervalHours ${this.refreshIntervalHours} is not a positive number; using 24`); + return 24 * 60 * 60 * 1000; + } + const ms = hours * 60 * 60 * 1000; + if (ms > MAX_TIMER_MS) { + console.warn(`VSAC: refreshIntervalHours ${hours} is longer than a timer can wait; using ${Math.floor(MAX_TIMER_MS / 3600000)} hours`); + return MAX_TIMER_MS; + } + return ms; + } + /** * Stop the refresh timer (for cleanup) */ diff --git a/tx/workers/batch-validate.js b/tx/workers/batch-validate.js index e209a8a0..a12e1fd6 100644 --- a/tx/workers/batch-validate.js +++ b/tx/workers/batch-validate.js @@ -105,6 +105,10 @@ class BatchValidateWorker extends TerminologyWorker { for (const p of params.parameter) { if (p.name == 'validation') { + // Validating an entry never yields the event loop by itself, and IG builds send batches + // of up to 1,000 entries (~1 s of compute): give other requests a turn between entries + // (checkAndYield yields only after YIELD_INTERVAL ms of compute, so small batches don't). + await this.opContext.checkAndYield('batch-validate'); let op = new Parameters(); op.jsonObj.parameter = []; for (const gp of globalParams) { diff --git a/tx/workers/expand.js b/tx/workers/expand.js index a9124096..b994df74 100644 --- a/tx/workers/expand.js +++ b/tx/workers/expand.js @@ -221,6 +221,18 @@ class ValueSetExpander { this.csCounter = new Map(); } + /** + * The value set as a message names it: its url, or - for a value set in the request, which + * needn't have one - its id, or '??' (the text the tx-ecosystem tests expect) + */ + valueSetName(url) { + if (url) { + return url; + } + const id = this.valueSet && this.valueSet.jsonObj ? this.valueSet.jsonObj.id : null; + return id ? id : '??'; + } + async listDisplaysFromProvider(displays, cs, context) { await cs.designations(context, displays, true); displays.source = cs; @@ -349,10 +361,7 @@ class ValueSetExpander { this.noTotal(); throw new Issue('information', 'informational', null, null, null, null).setFinished(); } else { - if (!srcURL) { - srcURL = '??'; - } - throw new Issue("error", "too-costly", null, 'VALUESET_TOO_COSTLY', this.worker.i18n.translate('VALUESET_TOO_COSTLY', this.params.httpLanguages, [srcURL, '>' + this.limitCount]), null, 422).withDiagnostics(this.worker.opContext.diagnostics()); + throw new Issue("error", "too-costly", null, 'VALUESET_TOO_COSTLY', this.worker.i18n.translate('VALUESET_TOO_COSTLY', this.params.httpLanguages, [this.valueSetName(srcURL), '>' + this.limitCount]), null, 422).withDiagnostics(this.worker.opContext.diagnostics()); } } @@ -533,10 +542,7 @@ class ValueSetExpander { if (this.count > -1 && this.offset > -1 && this.count + this.offset > 0 && this.fullList.length >= this.count + this.offset) { throw new Issue('information', 'informational', null, null, null, null).setFinished(); } else { - if (!srcURL) { - srcURL = '??'; - } - throw new Issue("error", "too-costly", null, 'VALUESET_TOO_COSTLY', this.worker.i18n.translate('VALUESET_TOO_COSTLY', this.params.httpLanguages, [srcURL, '>' + this.limitCount]), null, 422).withDiagnostics(this.worker.opContext.diagnostics()); + throw new Issue("error", "too-costly", null, 'VALUESET_TOO_COSTLY', this.worker.i18n.translate('VALUESET_TOO_COSTLY', this.params.httpLanguages, [this.valueSetName(srcURL), '>' + this.limitCount]), null, 422).withDiagnostics(this.worker.opContext.diagnostics()); } } @@ -694,13 +700,14 @@ class ValueSetExpander { if (vsInfo && vsInfo.isSimple) { vsInfo.handleByCS = cs.handlesSelecting(); } + // A code system with no content to expand from is refused like an unknown one: 422 if (!cs.contentMode()) { - throw new Issue('error', 'business-rule', null, null, 'The code system definition for ' + cset.system + ' has no content property, so this expansion cannot be performed', 'invalid'); + throw new Issue('error', 'business-rule', null, null, 'The code system definition for ' + cset.system + ' has no content property, so this expansion cannot be performed', 'invalid', 422); } else if (cs.contentMode() !== 'complete') { if (cs.contentMode() === 'not-present') { - throw new Issue('error', 'business-rule', null, null, 'The code system definition for ' + cset.system + ' has no content, so this expansion cannot be performed', 'invalid'); + throw new Issue('error', 'business-rule', null, null, 'The code system definition for ' + cset.system + ' has no content, so this expansion cannot be performed', 'invalid', 422); } else if (cs.contentMode() === 'supplement') { - throw new Issue('error', 'business-rule', null, null, 'The code system definition for ' + cset.system + ' defines a supplement, so this expansion cannot be performed', 'invalid'); + throw new Issue('error', 'business-rule', null, null, 'The code system definition for ' + cset.system + ' defines a supplement, so this expansion cannot be performed', 'invalid', 422); } else if (cs.contentMode() === 'fragment') { this.addParamUri(exp, 'used-fragment', cs.system()+ (cs.version() ? '|' + cs.version(): "")); Extensions.addBoolean(exp, "http://hl7.org/fhir/StructureDefinition/valueset-unclosed", true); @@ -732,7 +739,7 @@ class ValueSetExpander { // Report the versioned url (vurl), matching the message the // enumeration path produces when it hits the same limit - this // guard is just that refusal moved earlier. - throw new Issue("error", "too-costly", null, 'VALUESET_TOO_COSTLY', this.worker.i18n.translate('VALUESET_TOO_COSTLY', this.params.httpLanguages, [source && source.vurl ? source.vurl : srcURL, '>' + this.limitCount]), null, 422).withDiagnostics(this.worker.opContext.diagnostics()); + throw new Issue("error", "too-costly", null, 'VALUESET_TOO_COSTLY', this.worker.i18n.translate('VALUESET_TOO_COSTLY', this.params.httpLanguages, [this.valueSetName(source && source.url ? source.vurl : srcURL), '>' + this.limitCount]), null, 422).withDiagnostics(this.worker.opContext.diagnostics()); } } } @@ -846,7 +853,7 @@ class ValueSetExpander { } else { const iter = await cs.iterator(null); if (valueSets.length === 0 && this.limitCount > 0 && (iter && iter.total > this.limitCount) && this.offset < 0) { - throw new Issue("error", "too-costly", null, 'VALUESET_TOO_COSTLY', this.worker.i18n.translate('VALUESET_TOO_COSTLY', this.params.httpLanguages, [vsSrc.vurl, '>' + this.limitCount]), null, 422).withDiagnostics(this.worker.opContext.diagnostics()); + throw new Issue("error", "too-costly", null, 'VALUESET_TOO_COSTLY', this.worker.i18n.translate('VALUESET_TOO_COSTLY', this.params.httpLanguages, [this.valueSetName(vsSrc.url ? vsSrc.vurl : null), '>' + this.limitCount]), null, 422).withDiagnostics(this.worker.opContext.diagnostics()); } let tcount = 0; @@ -1647,7 +1654,7 @@ class ValueSetExpander { if (this.offset + this.count < 0 && this.fullList.length > this.limit) { this.log.log('Operation took too long @ expand (' + this.constructor.name + ')'); - throw new Issue("error", "too-costly", null, 'VALUESET_TOO_COSTLY', this.worker.i18n.translate('VALUESET_TOO_COSTLY', this.params.httpLanguages, [source.vurl, '>' + this.limit]), null, 422).withDiagnostics(this.worker.opContext.diagnostics()); + throw new Issue("error", "too-costly", null, 'VALUESET_TOO_COSTLY', this.worker.i18n.translate('VALUESET_TOO_COSTLY', this.params.httpLanguages, [this.valueSetName(source.url ? source.vurl : null), '>' + this.limit]), null, 422).withDiagnostics(this.worker.opContext.diagnostics()); } else { let t = 0; let o = 0; diff --git a/tx/workers/validate.js b/tx/workers/validate.js index d4b5a8d6..df3d503e 100644 --- a/tx/workers/validate.js +++ b/tx/workers/validate.js @@ -1618,6 +1618,15 @@ class ValueSetChecker { return !!b.context && (await cs.code(a.context)) === (await cs.code(b.context)); } + /** + * True when the code has a second spelling at all (a classification code), so that an + * enumerated concept could be the same concept written differently. + */ + async hasOtherForm(cs, code) { + const a = await cs.locate(code); + return !!a.context && !!(await cs.classificationCode(a.context)); + } + async noteClassificationCode(cs, context, code, normalForm) { // the code system decides whether this spelling of the code earns a classificationCode // (ICD-11: when it was written with entity ids, in any order - not when it was written @@ -1727,7 +1736,9 @@ class ValueSetChecker { // an enumerated concept may be written in the other form (ICD-11 short code vs uri) let altMatch = null; - if (cset.concept && !cset.concept.some(cc => cc.code === code)) { + // whether the code has another form does not depend on the listed concept: ask it once + // here, not once per listed concept (a locate per listed code on every miss) + if (cset.concept && !cset.concept.some(cc => cc.code === code) && (await this.hasOtherForm(cs, code))) { for (const cc of cset.concept) { if (await this.sameConceptDifferentForm(cs, code, cc.code)) { altMatch = cc; @@ -2091,13 +2102,12 @@ class ValidateWorker extends TerminologyWorker { this.log.error(error); debugLog(error); if (error instanceof Issue) { - if (error.isHandleAsOO()) { - let oo = new OperationOutcome(); - oo.addIssue(error); - return res.status(error.statusCode || 500).json(oo.jsonObj); - } else { - // this is actually handled in the inner method - } + // Issues raised inside the validation are turned into a result by the inner method; + // one that reaches here (e.g. cache-id-unknown from setupAdditionalResources, before + // the inner try) must still be answered, or the request never completes. + let oo = new OperationOutcome(); + oo.addIssue(error); + return res.status(error.statusCode || 500).json(oo.jsonObj); } else { return res.status(error.statusCode || 500).json(outcomeFromError(error)); } diff --git a/tx/workers/worker.js b/tx/workers/worker.js index 8f9f3cfd..20590509 100644 --- a/tx/workers/worker.js +++ b/tx/workers/worker.js @@ -349,7 +349,14 @@ class TerminologyWorker { return supplements; } - for (const resource of this.additionalResources) { + // only the request's supplement CodeSystems matter here; derive them once per request (again + // if the list changes) instead of walking every request resource on every lookup + if (this._supplementSource !== this.additionalResources || this._supplementSourceLength !== this.additionalResources.length) { + this._supplementResources = this.additionalResources.filter((r) => r.resourceType === 'CodeSystem' && r instanceof CodeSystem && r.jsonObj.supplements); + this._supplementSource = this.additionalResources; + this._supplementSourceLength = this.additionalResources.length; + } + for (const resource of this._supplementResources) { this.deadCheck('loadSupplements'); if (resource.resourceType === 'CodeSystem' && resource instanceof CodeSystem) { const cs = resource;