From 48e416b679f095832c5b7ae8ac4c020be6c6987f Mon Sep 17 00:00:00 2001 From: Josh Mandel Date: Mon, 2 Nov 2026 07:45:32 -0600 Subject: [PATCH 01/12] validate: a code not in an enumerated include is located once, not once per listed code Before trying each listed concept as the same concept in another form (ICD-11's short code and entity id), checkConceptSet called sameConceptDifferentForm for every listed concept, and each call located the code to ask whether it has another form at all. That answer does not depend on the listed concept, so it is now asked once per include and the loop runs only when it is yes. Same answers. A miss against a 33,000-code VSAC RxNorm list used to get no answer within 600 s (a 1,185-code list took 99 s); a 300-code list in the new test located the code 301 times, now a handful. Co-Authored-By: Claude Opus 5.5 --- tests/tx/validate-enumerated-miss.test.js | 64 +++++++++++++++++++++++ tx/workers/validate.js | 13 ++++- 2 files changed, 76 insertions(+), 1 deletion(-) create mode 100644 tests/tx/validate-enumerated-miss.test.js 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/tx/workers/validate.js b/tx/workers/validate.js index 8fc01f0b..8d6d3763 100644 --- a/tx/workers/validate.js +++ b/tx/workers/validate.js @@ -1617,6 +1617,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 @@ -1734,7 +1743,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; From 0bf05fbfae6170d1649b4af399346b7ffdf2a272 Mon Sep 17 00:00:00 2001 From: Josh Mandel Date: Mon, 2 Nov 2026 09:20:57 -0600 Subject: [PATCH 02/12] expand: hand a whole compose to a provider that handles selecting processCodes was an empty stub, so no provider was ever handed a compose even though handlesSelecting()/processSelection() are declared. It now calls processSelection with the compose's includes and excludes and the request's offset and count. The provider returns the page and the total of the whole selection ({sets, total, notClosed}), or null to decline, and then the compose takes the usual include/exclude path. Composes with a text filter, an enumerated concept list, or a filter with no value are not handed over. scanValueSet now records the compose's system and version (processCodes needs them) and reads include.valueSet (it read the lowercase spelling, which never matched), so a compose that imports a value set is not called simple. The up-front size guard in checkSource is skipped when the provider will page the compose itself. Main's providers return handlesSelecting() === false, so nothing changes for them. Co-Authored-By: Claude Opus 5.5 --- tests/tx/expand-process-selection.test.js | 176 ++++++++++++++++++++++ tx/cs/cs-api.js | 33 +++- tx/cs/cs-api.md | 21 ++- tx/workers/expand.js | 100 +++++++++++- 4 files changed, 311 insertions(+), 19 deletions(-) create mode 100644 tests/tx/expand-process-selection.test.js diff --git a/tests/tx/expand-process-selection.test.js b/tests/tx/expand-process-selection.test.js new file mode 100644 index 00000000..cb28b269 --- /dev/null +++ b/tests/tx/expand-process-selection.test.js @@ -0,0 +1,176 @@ +/** + * $expand hands a whole compose to a code system provider that handles selecting + * (CodeSystemProvider.handlesSelecting / processSelection; see ValueSetExpander.processCodes). + * + * The provider here is the ordinary FHIR CodeSystem provider, told to handle selecting for one + * test code system only. Its processSelection selects from the CodeSystem's concepts (an include + * of the whole system, an exclude with a `concept in` filter), applies offset and count, and + * reports the total - or declines (null), in which case the usual include/exclude path must give + * the same expansion as a provider that doesn't handle selecting at all. + */ +const request = require('supertest'); +const { getTestApp, shutdownTestApp } = require('./setup'); +const { FhirCodeSystemProvider, FhirCodeSystemProviderFilterContext } = require('../../tx/cs/cs-cs'); + +const CS_URL = 'http://example.org/fhir/CodeSystem/process-selection-cs'; +const N = 12; +const codes = Array.from({ length: N }, (_, i) => 'c' + String(i).padStart(2, '0')); +const CS = { + resourceType: 'CodeSystem', url: CS_URL, version: '1.0.0', status: 'active', content: 'complete', + concept: codes.map(code => ({ code, display: 'Display ' + code })) +}; + +let vsCounter = 0; +function valueSet(compose) { + vsCounter++; + return { + resourceType: 'ValueSet', url: 'http://example.org/fhir/ValueSet/process-selection-' + vsCounter, + version: '1.0.0', status: 'active', compose + }; +} +const wholeSystem = { system: CS_URL }; +const excludeTwo = { system: CS_URL, filter: [{ property: 'concept', op: 'in', value: 'c01,c02' }] }; + +async function expand(app, vs, extra = []) { + const res = await request(app).post('/tx/r5/ValueSet/$expand') + .set('Content-Type', 'application/json') + .send({ resourceType: 'Parameters', parameter: [ + { name: 'tx-resource', resource: CS }, + { name: 'valueSet', resource: vs }, + ...extra + ] }); + expect(res.status).toBe(200); + return res.body; +} +const codesOf = (vs) => (vs.expansion.contains || []).map(c => c.code); + +/** the selection a provider would make for this test code system (filters: `concept in` only) */ +function select(includes, excludes) { + const inFilter = (cset) => { + const f = (cset.filter || [])[0]; + return f ? new Set(f.value.split(',')) : null; + }; + const out = []; + for (const code of codes) { + const inc = includes.some(cset => { const s = inFilter(cset); return s == null || s.has(code); }); + const exc = excludes.some(cset => { const s = inFilter(cset); return s == null || s.has(code); }); + if (inc && !exc) { + out.push(code); + } + } + return out; +} + +describe('$expand: handing a whole compose to the provider (processSelection)', () => { + let app; + let calls; + let behaviour; // 'handle' | 'decline' | 'no-total' + const spies = []; + + beforeAll(async () => { + app = await getTestApp(); + const origHandles = FhirCodeSystemProvider.prototype.handlesSelecting; + spies.push(jest.spyOn(FhirCodeSystemProvider.prototype, 'handlesSelecting').mockImplementation(function () { + return this.system() === CS_URL && behaviour !== 'off' ? true : origHandles.call(this); + })); + spies.push(jest.spyOn(FhirCodeSystemProvider.prototype, 'processSelection').mockImplementation(async function (params, includes, excludes, excludeInactive, offset, count) { + calls.push({ includes, excludes, excludeInactive, offset, count }); + if (behaviour === 'decline') { + return null; + } + const all = select(includes, excludes); + const start = Math.max(offset, 0); + const page = count < 0 ? all.slice(start) : all.slice(start, start + count); + const set = new FhirCodeSystemProviderFilterContext(); + for (const code of page) { + set.add(this.codeSystem.getConceptByCode(code)); + } + return { sets: [set], total: behaviour === 'no-total' ? -1 : all.length }; + })); + }, 60000); + + afterAll(async () => { + spies.forEach(s => s.mockRestore()); + await shutdownTestApp(); + }); + + beforeEach(() => { + calls = []; + behaviour = 'handle'; + }); + + test('the provider gets the includes and excludes, and its codes and total make the expansion', async () => { + const vs = await expand(app, valueSet({ include: [wholeSystem], exclude: [excludeTwo] })); + expect(calls).toHaveLength(1); + expect(calls[0].includes).toEqual([wholeSystem]); + expect(calls[0].excludes).toEqual([excludeTwo]); + expect(calls[0].offset).toBe(-1); + expect(calls[0].count).toBe(-1); + expect(codesOf(vs)).toEqual(codes.filter(c => c !== 'c01' && c !== 'c02')); + expect(vs.expansion.total).toBe(N - 2); + expect(vs.expansion.contains[0].display).toBe('Display c00'); + expect(vs.expansion.parameter.some(p => p.name === 'used-codesystem' && p.valueUri === CS_URL + '|1.0.0')).toBe(true); + }); + + test('paging: the provider gets offset and count, and the page it returns is not paged again', async () => { + const vs = await expand(app, valueSet({ include: [wholeSystem], exclude: [excludeTwo] }), + [{ name: 'offset', valueInteger: 3 }, { name: 'count', valueInteger: 4 }]); + expect(calls).toHaveLength(1); + expect(calls[0].offset).toBe(3); + expect(calls[0].count).toBe(4); + expect(codesOf(vs)).toEqual(['c05', 'c06', 'c07', 'c08']); + expect(vs.expansion.total).toBe(N - 2); + expect(vs.expansion.offset).toBe(3); + }); + + test('count=0: the total only', async () => { + const vs = await expand(app, valueSet({ include: [wholeSystem] }), [{ name: 'count', valueInteger: 0 }]); + expect(calls[0].count).toBe(0); + expect(vs.expansion.contains).toBeUndefined(); + expect(vs.expansion.total).toBe(N); + }); + + test('a total the provider has not counted (-1) leaves the expansion without one', async () => { + behaviour = 'no-total'; + const vs = await expand(app, valueSet({ include: [wholeSystem] }), [{ name: 'count', valueInteger: 5 }]); + expect(codesOf(vs)).toEqual(codes.slice(0, 5)); + expect(vs.expansion.total).toBeUndefined(); + }); + + test('declined (null): the usual path gives the same expansion as a provider that does not handle selecting', async () => { + const compose = { include: [wholeSystem], exclude: [excludeTwo] }; + const paging = [{ name: 'offset', valueInteger: 2 }, { name: 'count', valueInteger: 5 }]; + behaviour = 'decline'; + const declined = await expand(app, valueSet(compose), paging); + expect(calls).toHaveLength(1); + behaviour = 'off'; + const usual = await expand(app, valueSet(compose), paging); + expect(codesOf(declined)).toEqual(codesOf(usual)); + expect(codesOf(declined)).toEqual(['c04', 'c05', 'c06', 'c07', 'c08']); + expect(declined.expansion.total).toEqual(usual.expansion.total); + }); + + test('handled gives the same codes as the usual path', async () => { + const compose = { include: [{ system: CS_URL, filter: [{ property: 'concept', op: 'in', value: 'c03,c04,c05,c09' }] }], exclude: [excludeTwo] }; + const handled = await expand(app, valueSet(compose)); + expect(calls).toHaveLength(1); + behaviour = 'off'; + const usual = await expand(app, valueSet(compose)); + expect(codesOf(handled)).toEqual(codesOf(usual)); + expect(handled.expansion.total).toEqual(usual.expansion.total); + }); + + test.each([ + ['a text filter', { include: [wholeSystem] }, [{ name: 'filter', valueString: 'c03' }]], + ['an enumerated concept list', { include: [{ system: CS_URL, concept: [{ code: 'c03' }, { code: 'c04' }] }] }, []], + ['a value set import', { include: [{ system: CS_URL, valueSet: ['http://example.org/fhir/ValueSet/none'] }] }, []], + ['a second code system', { include: [wholeSystem, { system: 'http://hl7.org/fhir/administrative-gender' }] }, []] + ])('%s: the compose is not handed over', async (_label, compose, extra) => { + await request(app).post('/tx/r5/ValueSet/$expand') + .set('Content-Type', 'application/json') + .send({ resourceType: 'Parameters', parameter: [ + { name: 'tx-resource', resource: CS }, { name: 'valueSet', resource: valueSet(compose) }, ...extra + ] }); + expect(calls).toHaveLength(0); + }); +}); diff --git a/tx/cs/cs-api.js b/tx/cs/cs-api.js index b3795410..ac41c22c 100644 --- a/tx/cs/cs-api.js +++ b/tx/cs/cs-api.js @@ -664,15 +664,32 @@ class CodeSystemProvider { } /** - * Process a set of includes and excludes for the code system + * Select the codes of a whole compose in one call (only called when handlesSelecting() is true). * - * @param {TxParameters} params: information from the request that the user made, to help optimise loading - * @param {Object[]} includes - a list of includes from the code system. Each include may contain just the system(+version), concepts and/or filters (but won't contain value sets) - * @param {Object[]} excludes - a list of excludes from the code system. Each include may contain just the system(+version), concepts and/or filters (but won't contain value sets) - * @param {boolean} excludeInactive: whether the server will use inactive codes or not - * @param {int} offset if handlesOffset() and !iterate, and if the value set is a simple one that only uses this provider, then this is the applicable offset. -1 if not applicable - * @param {int} count if handlesOffset() and !iterate, and if the value set is a simple one that only uses this provider, then this is the applicable count. -1 if not applicable - * @returns {FilterConceptSet[]} filter sets. In general, it wouldn't make sense to return more than one, but providers can do if they want to. See futher comments on executeFilters + * expand.js hands over a compose that selects from this code system alone (one system|version), + * imports no value sets, has no text filter, and lists no concepts (an enumerated concept + * carries displays and extensions from the value set, so those go through the usual path, + * where prefetchCodes() applies). Anything else this provider doesn't support - an + * unknown filter, say - it declines by returning null, and the compose goes through the + * usual include/exclude path (getPrepContext/filter/executeFilters, locate) instead. + * + * The provider applies offset and count: the sets it returns hold just the requested page of + * the selection, in expansion order. The worker adds every code in them to the expansion + * (with displays, properties etc. from this provider, as for a filter set), and takes the + * total from the result. + * + * @param {TxParameters} params: information from the request that the user made, to help optimise loading (activeOnly, properties, designations...) + * @param {Object[]} includes - the compose's includes. Each names this system (and version), with or without filters + * @param {Object[]} excludes - the compose's excludes, in the same form + * @param {boolean} excludeInactive: whether the value set leaves out inactive codes (compose.inactive = false) + * @param {int} offset the index of the first code to return, -1 when the request gave no offset (start at 0) + * @param {int} count the most codes to return, -1 when the request gave no count (all of them); 0 asks for the total only + * @returns {Promise<{sets: FilterConceptSet[], total: number, notClosed?: boolean}|null>} null to decline. + * Otherwise `sets` (in general one; iterated with filterMore/filterConcept) hold the page, + * `total` is the number of codes in the whole selection before paging, or -1 when the + * provider hasn't counted it (the expansion then has no total), and `notClosed` is true + * when the selection is open-ended (a grammar-based code system), so the expansion is + * marked unclosed */ processSelection(params, includes, excludes, excludeInactive, offset, count) { // well, you only need to override if handlesSelecting=true, but that's the only time this will be called diff --git a/tx/cs/cs-api.md b/tx/cs/cs-api.md index da57c309..1f48fb09 100644 --- a/tx/cs/cs-api.md +++ b/tx/cs/cs-api.md @@ -190,9 +190,24 @@ For large code systems where a value set selects codes from only that system, th hand the entire include/exclude list to the provider in one call rather than using the filter workflow above. -Override `handlesSelecting()` to return `true` and then implement `processSelection()`. -The method receives the full lists of includes and excludes, along with pagination parameters -if applicable, and returns `FilterConceptSet[]`. +Override `handlesSelecting()` to return `true` and then implement +`processSelection(params, includes, excludes, excludeInactive, offset, count)`. + +- **When it is called.** `$expand` hands over a compose that selects from this code system + alone (one system|version), imports no value sets, has no text filter and lists no concepts. + Value sets that list concepts take the usual path (see `prefetchCodes` under Lookup). +- **Declining.** Return `null` for anything the provider doesn't support (an unknown filter, + say). The compose then goes through the filter workflow above, as for any other provider. +- **Paging.** The provider applies `offset` (-1: none given, start at 0) and `count` + (-1: none given, return all; 0: the total only). The sets it returns hold just that page, + in expansion order. +- **Result.** `{ sets, total, notClosed }`: `sets` is a `FilterConceptSet[]` (in general one), + iterated with `filterMore`/`filterConcept`; `total` is the size of the whole selection + before paging, or -1 if the provider hasn't counted it (the expansion then has no total); + `notClosed` marks an open-ended selection (a grammar-based code system). + +The default `handlesSelecting()` returns `false`, so a provider that doesn't override it is +never asked. --- diff --git a/tx/workers/expand.js b/tx/workers/expand.js index 606de1c9..32239794 100644 --- a/tx/workers/expand.js +++ b/tx/workers/expand.js @@ -715,7 +715,9 @@ class ValueSetExpander { // NB: totalCount is a method - as a property read (cs.totalCount) the // comparison was against the function object, always false, so this // guard was silently dead. - if (!imp && this.count !== 0 && this.limitCount > 0 && await cs.totalCount() > this.limitCount + // (not when the provider will be handed the compose: it pages and counts the + // selection itself - see processCodes) + if (!imp && !(vsInfo && vsInfo.handleByCS) && this.count !== 0 && this.limitCount > 0 && await cs.totalCount() > this.limitCount && !(this.count > 0 && this.count + Math.max(this.offset, 0) <= this.limitCount)) { // Report the versioned url (vurl), matching the message the // enumeration path produces when it hits the same limit - this @@ -728,15 +730,90 @@ class ValueSetExpander { } } + /** + * Hand the whole compose to the code system provider (CodeSystemProvider.handlesSelecting / + * processSelection). Only called for a compose that selects from one system|version and imports + * no value sets (scanValueSet: isSimple), when the provider says it handles selecting. + * + * Returns true when the provider took the compose: its codes are in the expansion, already + * paged by the provider, with the total it reported. Returns false, having added nothing, when + * the compose is one this hand-off doesn't cover (a text filter, an enumerated concept list, a + * filter with no value, content that isn't complete) or the provider declines by returning + * null: the compose then goes through the usual include/exclude path. + */ async processCodes(path, vsSrc, compose, filter, expansion, excludeInactive, notClosed, vsInfo) { + const includes = compose.include || []; + const excludes = compose.exclude || []; + if (!filter.isNull) { + // the provider isn't told about a text filter + return false; + } + if ([...includes, ...excludes].some(cset => cset.concept)) { + // an enumerated concept carries displays, designations and extensions from the value set + return false; + } + if ([...includes, ...excludes].some(cset => (cset.filter || []).some(fc => !fc.value))) { + // the usual path reports the error + return false; + } const cs = await this.worker.findCodeSystem(vsInfo.system, vsInfo.version, this.params, ['complete', 'fragment'], false, false, true, null, this.requiredSupplements); - if (cs != null) { + if (cs == null || cs.contentMode() !== 'complete') { + return false; + } + for (const cset of [...includes, ...excludes]) { + Extensions.checkNoModifiers(cset, 'ValueSetExpander.processCodes', 'set', vsSrc.vurl); + for (const fc of cset.filter || []) { + Extensions.checkNoModifiers(fc, 'ValueSetExpander.processCodes', 'filter', vsSrc.vurl); + } + } + + const selection = await cs.processSelection(this.params, includes, excludes, excludeInactive, this.offset, this.count); + if (selection == null) { + return false; + } + this.worker.opContext.log('compose handled by the code system provider'); - // set up the call to the provider - // call the provider - // include the codes + for (const cset of [...includes, ...excludes]) { + this.worker.checkSupplements(cs, cset, this.requiredSupplements, this.usedSupplements); + } + this.checkProviderCanonicalStatus(expansion, cs, this.valueSet); + this.addParamUri(expansion, 'used-codesystem', this.canonical(cs.system(), cs.version())); + if (selection.notClosed) { + notClosed.value = true; + const wholeSystem = includes.some(cset => !(cset.filter || []).length); + if (wholeSystem && cs.isNotClosed() && !Extensions.has(expansion, "http://hl7.org/fhir/StructureDefinition/valueset-unclosed-reason")) { + // as the usual path says for a whole grammar-based code system + Extensions.addString(expansion, "http://hl7.org/fhir/StructureDefinition/valueset-unclosed-reason", "The code System '" + cs.system() + "' has a grammar and so has infinite members"); + } } + + // the provider has applied offset and count: every code it returns is in the page + vsInfo.csDoOffset = true; + this.canBeHierarchy = false; + const cds = new Designations(this.worker.i18n.languageDefinitions); + for (const set of selection.sets || []) { + while (await cs.filterMore(null, set)) { + await this.worker.checkAndYield('processCodes#selection'); + const c = await cs.filterConcept(null, set); + cds.clear(); + if (this.noDetails) { + await this.includeCode(cs, null, cs.system(), cs.version(), await cs.code(c), await cs.isAbstract(c), await cs.isInactive(c), + null, null, cds, null, null, expansion, null, null, null, null, null, excludeInactive, vsSrc.url); + } else { + await this.listDisplaysFromProvider(cds, cs, c); + await this.includeCode(cs, null, cs.system(), cs.version(), await cs.code(c), await cs.isAbstract(c), await cs.isInactive(c), + await cs.isDeprecated(c), await cs.getStatus(c), cds, await cs.definition(c), await cs.itemWeight(c), + expansion, null, await cs.extensions(c), null, await cs.properties(c), null, excludeInactive, vsSrc.url); + } + } + } + if (selection.total != null && selection.total >= 0) { + this.addToTotal(selection.total); + } else { + this.noTotal(); + } + return true; } async includeCodes(cset, path, vsSrc, compose, filter, expansion, excludeInactive, notClosed) { @@ -1315,8 +1392,8 @@ class ValueSetExpander { this.worker.opContext.log('compose #2'); - if (vsInfo.handleByCS) { - await this.processCodes("ValueSet.compose", source, source.jsonObj.compose, filter, expansion, this.excludeInactives(source), notClosed, vsInfo); + if (vsInfo.handleByCS && await this.processCodes("ValueSet.compose", source, source.jsonObj.compose, filter, expansion, this.excludeInactives(source), notClosed, vsInfo)) { + // the code system provider selected the codes (processSelection) } else { this.checkForExclusionVersionSpecialCase(source, expansion); @@ -1913,13 +1990,20 @@ class ValueSetExpander { } if (simple && result.csset.size == 1) { result.isSimple = true; + const first = (compose.include || [])[0] || (compose.exclude || [])[0]; + if (first) { + result.system = first.system; + result.version = first.version; + } } return result; } isSimpleSelect(inc, set) { set.add(inc.system+"|"+inc.version); - return !inc.valueset || inc.valueset.length == 0; + // (the element is valueSet: the lowercase spelling never matched, which was harmless only + // while processCodes did nothing) + return !!inc.system && (!inc.valueSet || inc.valueSet.length == 0); } excludeFilterList(exc) { From 77cd925d5b80817fe50b15dfbf27d19eff148383 Mon Sep 17 00:00:00 2001 From: Josh Mandel Date: Mon, 2 Nov 2026 09:21:41 -0600 Subject: [PATCH 03/12] cs-api: prefetchCodes, a hint before the worker locates a list of codes $expand locates each concept an include or exclude lists, and $validate-code locates the listed codes when it compares them with a code in another form. For main's in-memory providers each locate is a memory read; for a database-backed provider each is a round trip, and nothing said a run of codes was coming. prefetchCodes(codes, withDesignations) is called once with the whole list before those loops, so such a provider can read them together. The default does nothing. Co-Authored-By: Claude Opus 5.5 --- tests/tx/prefetch-codes.test.js | 101 ++++++++++++++++++++++++++++++++ tx/cs/cs-api.js | 13 ++++ tx/cs/cs-api.md | 1 + tx/workers/expand.js | 4 ++ tx/workers/validate.js | 2 + 5 files changed, 121 insertions(+) create mode 100644 tests/tx/prefetch-codes.test.js diff --git a/tests/tx/prefetch-codes.test.js b/tests/tx/prefetch-codes.test.js new file mode 100644 index 00000000..b1ee2d4c --- /dev/null +++ b/tests/tx/prefetch-codes.test.js @@ -0,0 +1,101 @@ +/** + * CodeSystemProvider.prefetchCodes: before the worker locates each code a value set lists, the + * provider is told the whole list, once - so a provider that reads from a database can read them + * together. The default does nothing, so the answers are the same with or without it. + */ +const request = require('supertest'); +const { getTestApp, shutdownTestApp } = require('./setup'); +const { FhirCodeSystemProvider } = require('../../tx/cs/cs-cs'); + +const CS_URL = 'http://example.org/fhir/CodeSystem/prefetch-codes-cs'; +const codes = ['a1', 'a2', 'a3', 'a4', 'a5']; +const CS = { + resourceType: 'CodeSystem', url: CS_URL, version: '1.0.0', status: 'active', content: 'complete', + concept: [...codes, 'other'].map(code => ({ code, display: 'Display ' + code })) +}; + +function post(app, op, parameter) { + return request(app).post('/tx/r5/ValueSet/' + op) + .set('Content-Type', 'application/json') + .send({ resourceType: 'Parameters', parameter: [{ name: 'tx-resource', resource: CS }, ...parameter] }); +} + +describe('CodeSystemProvider.prefetchCodes', () => { + let app; + let events; + const spies = []; + + beforeAll(async () => { + app = await getTestApp(); + const origPrefetch = FhirCodeSystemProvider.prototype.prefetchCodes; + const origLocate = FhirCodeSystemProvider.prototype.locate; + spies.push(jest.spyOn(FhirCodeSystemProvider.prototype, 'prefetchCodes').mockImplementation(function (list, withDesignations) { + if (this.system() === CS_URL) { + events.push({ prefetch: [...list], withDesignations }); + } + return origPrefetch.call(this, list, withDesignations); + })); + spies.push(jest.spyOn(FhirCodeSystemProvider.prototype, 'locate').mockImplementation(function (code, ...rest) { + if (this.system() === CS_URL) { + events.push({ locate: code }); + } + return origLocate.call(this, code, ...rest); + })); + }, 60000); + + afterAll(async () => { + spies.forEach(s => s.mockRestore()); + await shutdownTestApp(); + }); + + beforeEach(() => { + events = []; + }); + + test('$expand: an include that lists codes - the list once, before the locates', async () => { + const vs = { resourceType: 'ValueSet', url: 'http://example.org/fhir/ValueSet/prefetch-1', status: 'active', + compose: { include: [{ system: CS_URL, concept: codes.map(code => ({ code })) }] } }; + const res = await post(app, '$expand', [{ name: 'valueSet', resource: vs }]); + expect(res.status).toBe(200); + expect(res.body.expansion.contains.map(c => c.code)).toEqual(codes); + + const prefetches = events.filter(e => e.prefetch); + expect(prefetches).toEqual([{ prefetch: codes, withDesignations: true }]); + const first = events.findIndex(e => e.prefetch); + const firstLocate = events.findIndex(e => e.locate); + expect(first).toBeLessThan(firstLocate); + expect(events.filter(e => e.locate).map(e => e.locate)).toEqual(codes); + }); + + test('$expand: an exclude that lists codes - the list once, before the locates', async () => { + const vs = { resourceType: 'ValueSet', url: 'http://example.org/fhir/ValueSet/prefetch-2', status: 'active', + compose: { include: [{ system: CS_URL }], exclude: [{ system: CS_URL, concept: [{ code: 'a2' }, { code: 'a4' }] }] } }; + const res = await post(app, '$expand', [{ name: 'valueSet', resource: vs }]); + expect(res.status).toBe(200); + expect(res.body.expansion.contains.map(c => c.code)).toEqual(['a1', 'a3', 'a5', 'other']); + + expect(events.filter(e => e.prefetch)).toEqual([{ prefetch: ['a2', 'a4'], withDesignations: false }]); + expect(events.findIndex(e => e.prefetch)).toBeLessThan(events.findIndex(e => e.locate === 'a2')); + }); + + test('$validate-code: before the listed codes are compared with a code that is not listed', async () => { + // sameConceptDifferentForm locates listed codes only for a code system with two spellings of a + // code (ICD-11): make this one say it has them, so the listed codes are located + const spy = jest.spyOn(FhirCodeSystemProvider.prototype, 'classificationCode').mockImplementation(async function () { + return this.system() === CS_URL ? 'x' : null; + }); + try { + const vs = { resourceType: 'ValueSet', url: 'http://example.org/fhir/ValueSet/prefetch-3', status: 'active', + compose: { include: [{ system: CS_URL, concept: codes.map(code => ({ code })) }] } }; + const res = await post(app, '$validate-code', [{ name: 'valueSet', resource: vs }, + { name: 'coding', valueCoding: { system: CS_URL, code: 'other' } }]); + expect(res.status).toBe(200); + expect(res.body.parameter.find(p => p.name === 'result').valueBoolean).toBe(false); + + expect(events.filter(e => e.prefetch)).toEqual([{ prefetch: codes, withDesignations: false }]); + expect(events.findIndex(e => e.prefetch)).toBeLessThan(events.findIndex(e => e.locate === 'a1')); + } finally { + spy.mockRestore(); + } + }); +}); diff --git a/tx/cs/cs-api.js b/tx/cs/cs-api.js index ac41c22c..5cd2ca8b 100644 --- a/tx/cs/cs-api.js +++ b/tx/cs/cs-api.js @@ -696,6 +696,19 @@ class CodeSystemProvider { throw new Error("Must override"); } + /** + * A hint, before the worker locates each code of a list one at a time: the concepts a value set + * include or exclude lists ($expand), or that validate-code compares a code with. A provider + * that answers locate() from a database may read the codes together here, and answer the + * locate() calls that follow from what it read, for this request only. The default does + * nothing, and a provider that ignores the hint gets the same calls as before. + * + * @param {String[]} codes - the codes, in the order the worker will locate them (it may stop early) + * @param {boolean} withDesignations - whether the worker will also list each code's displays and designations + */ + // eslint-disable-next-line no-unused-vars + async prefetchCodes(codes, withDesignations) { } + /** * returns true if a filter is supported * diff --git a/tx/cs/cs-api.md b/tx/cs/cs-api.md index 1f48fb09..f4cb1c0b 100644 --- a/tx/cs/cs-api.md +++ b/tx/cs/cs-api.md @@ -219,6 +219,7 @@ Lookup supports the `$lookup` and `$validate-code` operations. |---|---| | `locate(code)` | Look up a code by string. Returns `{ context, message }`. If found, `context` is a `CodeSystemProviderContext` and `message` is `null`. If not found, `context` is `null` and `message` explains why. **Must override.** | | `locateIsA(code)` | Look up a code within the scope of a parent concept. Required only if `hasParents()` returns `true`. | +| `prefetchCodes(codes, withDesignations)` | A hint before the worker locates each code of a list (the concepts a value set include or exclude lists, in `$expand` and `$validate-code`). A database-backed provider may read them together and answer the `locate()` calls that follow from that read, for this request only. Default: does nothing. | | `extendLookup(ctxt, props, params)` | Called during `$lookup` to allow the provider to add extra properties to the response `Parameters`. The `props` array lists the property names requested by the client. | The server calls `locate()` first to confirm the code exists, then uses the returned context diff --git a/tx/workers/expand.js b/tx/workers/expand.js index 32239794..a18a0f68 100644 --- a/tx/workers/expand.js +++ b/tx/workers/expand.js @@ -973,6 +973,8 @@ class ValueSetExpander { this.worker.opContext.log('iterate concepts'); const cds = new Designations(this.worker.i18n.languageDefinitions); + // a hint: each listed code is located next (a provider may read them together) + await cs.prefetchCodes(cset.concept.map(cc => cc.code), !this.noDetails); for (const cc of cset.concept) { await this.worker.checkAndYield('processCodes#3'); cds.clear(); @@ -1204,6 +1206,8 @@ class ValueSetExpander { if (cset.concept) { this.worker.opContext.log('iterate concepts'); const cds = new Designations(this.worker.i18n.languageDefinitions); + // a hint: each listed code is located next (a provider may read them together) + await cs.prefetchCodes(cset.concept.map(cc => cc.code), false); for (const cc of cset.concept) { await this.worker.checkAndYield('processCodes#3'); cds.clear(); diff --git a/tx/workers/validate.js b/tx/workers/validate.js index 8d6d3763..a0381abe 100644 --- a/tx/workers/validate.js +++ b/tx/workers/validate.js @@ -1746,6 +1746,8 @@ class ValueSetChecker { // 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))) { + // a hint: each listed code is located next (a provider may read them together) + await cs.prefetchCodes(cset.concept.map(cc => cc.code), false); for (const cc of cset.concept) { if (await this.sameConceptDifferentForm(cs, code, cc.code)) { altMatch = cc; From 9ba94be69260b7dff14c30d3e44c8f3b57b64bca Mon Sep 17 00:00:00 2001 From: Josh Mandel Date: Mon, 2 Nov 2026 09:23:42 -0600 Subject: [PATCH 04/12] validate: ask a stored concept list about one code instead of reading it $validate-code walked an include's concept array to find the code. A value-set provider that keeps lists as rows (VSAC lists of 33,000 to 57,000 codes) would have to build that whole array on every request. An include may now carry a stored-list handle (tx/library/stored-concepts.js: {count, lookup(code)}); validate.js asks it for the entries with the code, in determineSystem and checkConceptSet, and the renderer's include summary shows the count instead of every code. The whole list is read only when the code may be listed in another form (ICD-11), after the once-per-include hasOtherForm question. An ordinary array behaves exactly as before. Co-Authored-By: Claude Opus 5.5 --- tests/tx/stored-concepts.test.js | 156 +++++++++++++++++++++++++++++++ tx/library/renderer.js | 6 +- tx/library/stored-concepts.js | 36 +++++++ tx/workers/validate.js | 25 +++-- 4 files changed, 215 insertions(+), 8 deletions(-) create mode 100644 tests/tx/stored-concepts.test.js create mode 100644 tx/library/stored-concepts.js diff --git a/tests/tx/stored-concepts.test.js b/tests/tx/stored-concepts.test.js new file mode 100644 index 00000000..8d53c9d9 --- /dev/null +++ b/tests/tx/stored-concepts.test.js @@ -0,0 +1,156 @@ +/** + * A value set include whose concept list is kept in a store (tx/library/stored-concepts.js): + * $validate-code asks the list about the one code (lookup) instead of reading every entry, and + * gives the same answers as for an ordinary array. + */ +const request = require('supertest'); +const { getTestApp, shutdownTestApp } = require('./setup'); +const { FhirCodeSystemProvider } = require('../../tx/cs/cs-cs'); +const { ValueSetChecker } = require('../../tx/workers/validate'); +const { STORED_CONCEPTS, storedList, hasConcepts, conceptsWithCode } = require('../../tx/library/stored-concepts'); + +const CS_URL = 'http://example.org/fhir/CodeSystem/stored-concepts-cs'; +const N = 50; +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', 'alt7'].map(code => ({ code, display: 'Display ' + code })) +}; +const VS_URL = 'http://example.org/fhir/ValueSet/stored-concepts-vs'; +const VS = { + resourceType: 'ValueSet', url: VS_URL, version: '1.0.0', status: 'active', + compose: { include: [{ system: CS_URL, concept: codes.map(code => ({ code, display: 'VS display ' + code })) }] } +}; + +/** turn an include into a stored list: `concept` reads (and counts) the list, the handle answers lookups */ +function makeStored(cset, stats) { + const list = cset.concept; + delete cset.concept; + Object.defineProperty(cset, 'concept', { + get() { stats.reads++; return list; }, enumerable: false, configurable: true + }); + cset[STORED_CONCEPTS] = { + count: list.length, + lookup: async (code) => { stats.lookups.push(code); return list.filter(cc => cc.code === code); } + }; +} + +describe('stored-concepts helpers', () => { + test('an ordinary include', async () => { + const inc = { system: 'x', concept: [{ code: 'a' }, { code: 'b' }, { code: 'a', display: 'again' }] }; + expect(storedList(inc)).toBeNull(); + expect(hasConcepts(inc)).toBe(true); + expect(hasConcepts({ system: 'x' })).toBe(false); + expect(hasConcepts({ system: 'x', concept: [] })).toBe(true); // as !!cset.concept + expect(await conceptsWithCode(inc, 'a')).toEqual([{ code: 'a' }, { code: 'a', display: 'again' }]); + expect(await conceptsWithCode({ system: 'x' }, 'a')).toEqual([]); + }); + + test('a stored include is asked, not read', async () => { + const stats = { reads: 0, lookups: [] }; + const inc = { system: 'x', concept: [{ code: 'a' }, { code: 'b' }] }; + makeStored(inc, stats); + expect(storedList(inc).count).toBe(2); + expect(hasConcepts(inc)).toBe(true); + expect(await conceptsWithCode(inc, 'b')).toEqual([{ code: 'b' }]); + expect(stats).toEqual({ reads: 0, lookups: ['b'] }); + }); +}); + +describe('$validate-code against a stored concept list', () => { + let app; + let stats; + let stored; // whether the next request's include is turned into a stored list + const spies = []; + + beforeAll(async () => { + app = await getTestApp(); + const origPrepare = ValueSetChecker.prototype.prepare; + spies.push(jest.spyOn(ValueSetChecker.prototype, 'prepare').mockImplementation(async function (...args) { + const compose = this.valueSet && this.valueSet.jsonObj.compose; + if (stored && compose && this.valueSet.url === VS_URL) { + for (const inc of compose.include) { + if (!storedList(inc)) { + makeStored(inc, stats); + } + } + } + return origPrepare.apply(this, args); + })); + }, 60000); + + afterAll(async () => { + spies.forEach(s => s.mockRestore()); + await shutdownTestApp(); + }); + + async function validate(coding, extra = []) { + const res = await 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: structuredClone(VS) }, + ...(coding ? [{ name: 'coding', valueCoding: coding }] : []), + ...extra + ] }); + expect(res.status).toBe(200); + const out = {}; + for (const p of res.body.parameter) { + if (p.name !== 'issues') { + out[p.name] = p.valueBoolean ?? p.valueString ?? p.valueCode ?? p.valueUri; + } + } + return out; + } + + /** the answer for an ordinary list, then for the stored list (and what the stored list was asked) */ + async function both(coding, extra) { + stored = false; + const plain = await validate(coding, extra); + stored = true; + stats = { reads: 0, lookups: [] }; + const fromStore = await validate(coding, extra); + return { plain, fromStore, stats }; + } + + test('a listed code: one lookup, the list is not read, same answer', async () => { + const { plain, fromStore, stats } = await both({ system: CS_URL, code: 'c25' }); + expect(plain.result).toBe(true); + expect(fromStore).toEqual(plain); + expect(stats.lookups).toEqual(['c25']); + expect(stats.reads).toBe(0); + }); + + test('a code that is not listed: the list is not read, same answer', async () => { + const { plain, fromStore, stats } = await both({ system: CS_URL, code: 'unlisted' }); + expect(plain.result).toBe(false); + expect(fromStore).toEqual(plain); + expect(stats.reads).toBe(0); + }); + + test('inferring the system from the value set asks the list too', async () => { + const { plain, fromStore, stats } = await both(null, [{ name: 'code', valueCode: 'c3' }, { name: 'inferSystem', valueBoolean: true }]); + expect(plain.result).toBe(true); + expect(plain.system).toBe(CS_URL); + expect(fromStore).toEqual(plain); + expect(stats.lookups).toContain('c3'); + expect(stats.reads).toBe(0); + }); + + test('a code listed in another form (as ICD-11 short codes and entity ids): the whole list is read, same answer', async () => { + // 'alt7' stands for the same concept as the listed 'c7' + const s1 = jest.spyOn(ValueSetChecker.prototype, 'hasOtherForm').mockImplementation(async (cs, code) => code === 'alt7'); + const s2 = jest.spyOn(ValueSetChecker.prototype, 'sameConceptDifferentForm').mockImplementation(async (cs, code, listed) => code === 'alt7' && listed === 'c7'); + const s3 = jest.spyOn(FhirCodeSystemProvider.prototype, 'prefetchCodes'); + try { + const { plain, fromStore, stats } = await both({ system: CS_URL, code: 'alt7' }); + expect(plain.result).toBe(true); + expect(fromStore).toEqual(plain); + expect(stats.reads).toBeGreaterThan(0); + } finally { + s1.mockRestore(); + s2.mockRestore(); + s3.mockRestore(); + } + }); +}); diff --git a/tx/library/renderer.js b/tx/library/renderer.js index 075ba373..65707656 100644 --- a/tx/library/renderer.js +++ b/tx/library/renderer.js @@ -4,6 +4,7 @@ const {div} = require("../../library/html"); const {getValuePrimitive} = require("../../library/utilities"); const {getValueName} = require("../../library/utilities"); const {InvalidError} = require("./errors"); +const { storedList } = require("./stored-concepts"); // Valid FHIR value sets used when validating values before rendering, so that // illegal input produces an error that names the offending value rather than @@ -112,7 +113,10 @@ class Renderer { let result; if (inc.system) { result = '(' + inc.system + ')'; - if (inc.concept) { + if (storedList(inc)) { + // a stored list (see stored-concepts.js) isn't read just to describe the include + result = result + '(' + storedList(inc).count + ' codes)'; + } else if (inc.concept) { result = result + '('; let first = true; for (const cc of inc.concept) { diff --git a/tx/library/stored-concepts.js b/tx/library/stored-concepts.js new file mode 100644 index 00000000..71d26d11 --- /dev/null +++ b/tx/library/stored-concepts.js @@ -0,0 +1,36 @@ +'use strict'; +// +// A value set include or exclude whose concept list a value-set provider keeps in a store (a +// database, say) rather than in memory. The provider puts a handle on the include under +// STORED_CONCEPTS: +// +// { count: number, lookup(code) => the listed concepts with this code, in list order } +// +// lookup may return a promise. The include's `concept` must still give the whole list when it is +// read (typically a getter that reads it on first use), for code that needs every entry; code +// that asks about one code uses the handle and doesn't read the list. An ordinary include, with +// `concept` an array and no handle, behaves as always. +// + +const STORED_CONCEPTS = Symbol.for('fhirsmith.storedConcepts'); + +/** the include's stored-list handle, or null for an ordinary include */ +function storedList(cset) { + return (cset && cset[STORED_CONCEPTS]) || null; +} + +/** whether the include lists concepts (as `!!cset.concept`), without reading a stored list */ +function hasConcepts(cset) { + return !!storedList(cset) || !!cset.concept; +} + +/** the concepts the include lists with this code, in list order */ +async function conceptsWithCode(cset, code) { + const stored = storedList(cset); + if (stored) { + return await stored.lookup(code); + } + return (cset.concept || []).filter(cc => cc.code === code); +} + +module.exports = { STORED_CONCEPTS, storedList, hasConcepts, conceptsWithCode }; diff --git a/tx/workers/validate.js b/tx/workers/validate.js index a0381abe..bc8308d7 100644 --- a/tx/workers/validate.js +++ b/tx/workers/validate.js @@ -25,6 +25,7 @@ const {FhirCodeSystemProvider} = require("../cs/cs-cs"); const {CodeSystem} = require("../library/codesystem"); const {VersionUtilities} = require("../../library/version-utilities"); const {debugLog} = require("../operation-context"); +const { storedList, hasConcepts, conceptsWithCode } = require('../library/stored-concepts'); const DEV_IGNORE_VALUESET = false; // todo: what's going on with this (ported from pascal) @@ -181,8 +182,9 @@ class ValueSetChecker { if (cs === null) { return ''; } - if (vsi.concept) { - for (let cc of vsi.concept) { + if (hasConcepts(vsi)) { + // (only the listed concepts with this code: a stored list is asked, not read) + for (let cc of await conceptsWithCode(vsi, code)) { this.worker.deadCheck('determineSystem#2'); let match = cc.code === code; if (match) { @@ -1651,7 +1653,7 @@ class ValueSetChecker { async checkConceptSet(path, role, cs, cset, code, displays, vs, message, inactive, normalForm, vstatus, op, vcc, messages) { this.worker.opContext.addNote(vs, 'check code ' + role + ' ' + this.worker.renderer.displayValueSetInclude(cset) + ' at ' + path, this.indentCount); let result = false; - if (!cset.concept && !cset.filter) { + if (!hasConcepts(cset) && !cset.filter) { let loc = await cs.locate(code); result = false; if (loc.context == null) { @@ -1743,19 +1745,28 @@ class ValueSetChecker { // an enumerated concept may be written in the other form (ICD-11 short code vs uri) let altMatch = null; + // the listed concepts that can match: all of an ordinary list (as always), or just the ones + // with this code from a stored list, which is asked rather than read + const stored = storedList(cset); + let listed = stored ? await stored.lookup(code) : cset.concept; // 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))) { + if (listed && !listed.some(cc => cc.code === code) && (await this.hasOtherForm(cs, code))) { + // the code may be listed in its other form: this needs the whole list + const all = cset.concept || []; // a hint: each listed code is located next (a provider may read them together) - await cs.prefetchCodes(cset.concept.map(cc => cc.code), false); - for (const cc of cset.concept) { + await cs.prefetchCodes(all.map(cc => cc.code), false); + for (const cc of all) { if (await this.sameConceptDifferentForm(cs, code, cc.code)) { altMatch = cc; break; } } + if (stored && altMatch) { + listed = [altMatch]; + } } - for (let cc of cset.concept || []) { + for (let cc of listed || []) { this.worker.deadCheck('checkConceptSet#1'); let c = cc.code; if (code === c || cc === altMatch) { From 7f868dee05abc16b2fe86405e80131cf473444e8 Mon Sep 17 00:00:00 2001 From: Josh Mandel Date: Mon, 2 Nov 2026 09:24:59 -0600 Subject: [PATCH 05/12] provider: ask value-set providers for THO precedence instead of reading their maps applyTHOPrecedence found THO's value sets by checking instanceof PackageValueSetProvider and walking its valueSetMap, and found the core package the same way. A value-set provider that keeps its value sets in a database has no such map. Value-set providers now answer two questions: thoVersions(map) adds the url -> versions of a THO package's value sets, and yieldToTHO(map) keeps only pinned references for a core package. The base class defaults do nothing, and PackageValueSetProvider's implementations are the old loop and the old check, so the result is the same. The startup log uses the existing vsCount() rather than valueSetMap.size (so it now shows the number of value sets, not of map keys). Co-Authored-By: Claude Opus 5.5 --- tests/tx/vs-provider-tho.test.js | 128 +++++++++++++++++++++++++++++++ tx/library.js | 4 +- tx/library/tho-precedence.js | 11 ++- tx/provider.js | 14 ++-- tx/vs/vs-api.js | 21 +++++ tx/vs/vs-package.js | 37 ++++++++- 6 files changed, 202 insertions(+), 13 deletions(-) create mode 100644 tests/tx/vs-provider-tho.test.js diff --git a/tests/tx/vs-provider-tho.test.js b/tests/tx/vs-provider-tho.test.js new file mode 100644 index 00000000..73b8b8ad --- /dev/null +++ b/tests/tx/vs-provider-tho.test.js @@ -0,0 +1,128 @@ +/** + * THO precedence asks each value-set provider (thoVersions, yieldToTHO) instead of reaching into + * PackageValueSetProvider's valueSetMap. The defaults give what the old loops gave: a THO + * package's value sets' url -> versions, and keepPinnedOnly for a core package only. A provider + * that keeps no map can take part by answering the same questions. + */ +const { Provider } = require('../../tx/provider'); +const { PackageValueSetProvider } = require('../../tx/vs/vs-package'); +const { AbstractValueSetProvider } = require('../../tx/vs/vs-api'); +const { isTHOPackage } = require('../../tx/library/tho-precedence'); + +const URL_A = 'http://terminology.hl7.org/ValueSet/a'; +const URL_B = 'http://terminology.hl7.org/ValueSet/b'; + +/** a PackageValueSetProvider with this map, as initialize() leaves it (keys: id, url, url|version) */ +function packageProvider(pid, valueSets) { + const p = Object.create(PackageValueSetProvider.prototype); + p.sourcePackageCode = pid; + p.valueSetMap = new Map(); + for (const vs of valueSets) { + p.valueSetMap.set(vs.id, vs); + p.valueSetMap.set(vs.url, vs); + if (vs.version) { + p.valueSetMap.set(vs.url + '|' + vs.version, vs); + } + } + p.keepPinnedOnly = jest.fn(); + return p; +} + +/** the loop provider.js ran before (instanceof PackageValueSetProvider + valueSetMap) */ +function oldThoVersions(providers) { + const map = new Map(); + for (const vp of providers) { + if (vp instanceof PackageValueSetProvider && isTHOPackage(vp.sourcePackage())) { + for (const vs of vp.valueSetMap.values()) { + if (!vs || !vs.url) { + continue; + } + if (!map.has(vs.url)) { + map.set(vs.url, new Set()); + } + if (vs.version) { + map.get(vs.url).add(vs.version); + } + } + } + } + return map; +} + +function newProvider(valueSetProviders) { + const p = Object.create(Provider.prototype); + p.valueSetProviders = valueSetProviders; + p.conceptMapProviders = []; + return p; +} + +const tho = () => packageProvider('hl7.terminology.r4#6.0.2', [ + { id: 'a', url: URL_A, version: '1.0.0' }, { id: 'a2', url: URL_A, version: '2.0.0' }, { id: 'b', url: URL_B } +]); +const core = () => packageProvider('hl7.fhir.r4.core#4.0.1', [{ id: 'a', url: URL_A, version: '4.0.1' }]); +const other = () => packageProvider('hl7.fhir.us.core#6.1.0', [{ id: 'x', url: 'http://example.org/x', version: '1' }]); + +describe('value-set providers answer the THO precedence questions', () => { + test('thoVersions gives what the old valueSetMap loop gave', () => { + const providers = [tho(), core(), other()]; + const map = new Map(); + for (const vp of providers) { + vp.thoVersions(map); + } + expect(map).toEqual(oldThoVersions(providers)); + expect(map).toEqual(new Map([[URL_A, new Set(['1.0.0', '2.0.0'])], [URL_B, new Set()]])); + }); + + test('a package that is not THO adds nothing', () => { + const map = new Map(); + core().thoVersions(map); + other().thoVersions(map); + expect(map.size).toBe(0); + }); + + test('yieldToTHO calls keepPinnedOnly for a core package only', () => { + const map = new Map([[URL_A, new Set(['1.0.0'])]]); + const c = core(); + const t = tho(); + const o = other(); + for (const vp of [c, t, o]) { + vp.yieldToTHO(map); + } + expect(c.keepPinnedOnly).toHaveBeenCalledWith(map); + expect(t.keepPinnedOnly).not.toHaveBeenCalled(); + expect(o.keepPinnedOnly).not.toHaveBeenCalled(); + }); + + test('the base class defaults do nothing', () => { + const vp = new AbstractValueSetProvider(); + const map = new Map(); + vp.thoVersions(map); + vp.yieldToTHO(map); + expect(map.size).toBe(0); + }); + + test('Provider.applyTHOPrecedence: core yields to THO, as before', () => { + const t = tho(); + const c = core(); + newProvider([c, t, other()]).applyTHOPrecedence(); + expect(c.keepPinnedOnly).toHaveBeenCalledTimes(1); + expect(c.keepPinnedOnly.mock.calls[0][0]).toEqual(new Map([[URL_A, new Set(['1.0.0', '2.0.0'])], [URL_B, new Set()]])); + }); + + test('Provider.applyTHOPrecedence: a provider with no map takes part through the methods', () => { + class RowsProvider extends AbstractValueSetProvider { + thoVersions(map) { + map.set(URL_A, new Set(['9.9.9'])); + } + } + const c = core(); + newProvider([c, new RowsProvider()]).applyTHOPrecedence(); + expect(c.keepPinnedOnly.mock.calls[0][0]).toEqual(new Map([[URL_A, new Set(['9.9.9'])]])); + }); + + test('vsCount, which the startup log now reports, counts value sets rather than map keys', () => { + const p = Object.create(PackageValueSetProvider.prototype); + p.database = { vsCount: 3 }; + expect(p.vsCount()).toBe(3); + }); +}); diff --git a/tx/library.js b/tx/library.js index 1e769d40..3bd3a8a1 100644 --- a/tx/library.js +++ b/tx/library.js @@ -839,7 +839,7 @@ class Library { this.conceptMapProviders.push(cm); } - this.#logPackage(contentLoader.id(), contentLoader.version(), csc, vs ? vs.valueSetMap.size : 0); + this.#logPackage(contentLoader.id(), contentLoader.version(), csc, vs ? vs.vsCount() : 0); } /** @@ -979,7 +979,7 @@ class Library { await cm.initialize(); this.conceptMapProviders.push(cm); - this.#logPackage(contentLoader.id(), contentLoader.version(), csc, vs.valueSetMap.size); + this.#logPackage(contentLoader.id(), contentLoader.version(), csc, vs.vsCount()); } /** diff --git a/tx/library/tho-precedence.js b/tx/library/tho-precedence.js index 39851b2c..6b79bc78 100644 --- a/tx/library/tho-precedence.js +++ b/tx/library/tho-precedence.js @@ -37,4 +37,13 @@ function keepForPinnedReference(key, resource, thoVersions) { return false; } -module.exports = { keepForPinnedReference }; +/** + * Whether a package is one of the hl7.terminology (THO) packages + * @param {string} pid - the package id (with or without #version) + * @returns {boolean} + */ +function isTHOPackage(pid) { + return pid != null && pid.startsWith("hl7.terminology"); +} + +module.exports = { keepForPinnedReference, isTHOPackage }; diff --git a/tx/provider.js b/tx/provider.js index c551072d..2773beb9 100644 --- a/tx/provider.js +++ b/tx/provider.js @@ -8,6 +8,7 @@ const {validateParameter, validateOptionalParameter, validateArrayParameter} = r const path = require("path"); const {PackageContentLoader} = require("../library/package-manager"); const {PackageValueSetProvider} = require("./vs/vs-package"); +const { isTHOPackage } = require("./library/tho-precedence"); const ValueSet = require("./library/valueset"); const {PackageConceptMapProvider} = require("./cm/cm-package"); const { applyIdPrefix, CORE_PREFIX } = require("./library/resource-ids"); @@ -622,7 +623,7 @@ class Provider { * terminology.hl7.org (THO/UTG) package. */ static #isTHOPackage(pid) { - return pid != null && pid.startsWith("hl7.terminology"); + return isTHOPackage(pid); } /** @@ -675,13 +676,10 @@ class Provider { * read by id, found by search, or used for translation unless asked for. */ applyTHOPrecedence() { + // each value set provider is asked (it may hold no map: a database-backed provider) const thoValueSets = new Map(); for (const vp of this.valueSetProviders) { - if (vp instanceof PackageValueSetProvider && Provider.#isTHOPackage(vp.sourcePackage())) { - for (const vs of vp.valueSetMap.values()) { - Provider.#addVersion(thoValueSets, vs); - } - } + vp.thoVersions(thoValueSets); } const thoConceptMaps = new Map(); for (const cmp of this.conceptMapProviders) { @@ -692,9 +690,7 @@ class Provider { } } for (const vp of this.valueSetProviders) { - if (vp instanceof PackageValueSetProvider && VersionUtilities.isCorePackage(vp.sourcePackage())) { - vp.keepPinnedOnly(thoValueSets); - } + vp.yieldToTHO(thoValueSets); } for (const cmp of this.conceptMapProviders) { if (cmp instanceof PackageConceptMapProvider && VersionUtilities.isCorePackage(cmp.sourcePackage())) { diff --git a/tx/vs/vs-api.js b/tx/vs/vs-api.js index f1892052..04786f30 100644 --- a/tx/vs/vs-api.js +++ b/tx/vs/vs-api.js @@ -120,6 +120,27 @@ class AbstractValueSetProvider { return []; } + /** + * THO precedence (Provider.applyTHOPrecedence), step 1: if this provider provides value sets + * from an hl7.terminology package, add each one's url and version to the map. The default adds + * nothing (PackageValueSetProvider adds the value sets of a THO package). + * + * @param {Map>} thoVersions - url -> the versions hl7.terminology provides; added to + */ + // eslint-disable-next-line no-unused-vars + thoVersions(thoVersions) { } + + /** + * THO precedence (Provider.applyTHOPrecedence), step 2: if this provider provides the FHIR core + * package's value sets, those with urls THO also provides are only available to a reference + * that names their version. The default does nothing (PackageValueSetProvider does this for a + * core package: keepPinnedOnly). + * + * @param {Map>} thoVersions - url -> the versions hl7.terminology provides + */ + // eslint-disable-next-line no-unused-vars + yieldToTHO(thoVersions) { } + async close() { } diff --git a/tx/vs/vs-package.js b/tx/vs/vs-package.js index 8f349344..25752f5c 100644 --- a/tx/vs/vs-package.js +++ b/tx/vs/vs-package.js @@ -8,7 +8,7 @@ const { checkContained } = require('../library/canonical-resource'); const { VersionUtilities } = require('../../library/version-utilities'); const {validateParameter} = require("../../library/utilities"); const { applyIdPrefix, stripIdPrefix } = require('../library/resource-ids'); -const { keepForPinnedReference } = require('../library/tho-precedence'); +const { keepForPinnedReference, isTHOPackage } = require('../library/tho-precedence'); /** * Package-based ValueSet provider using shared database layer @@ -461,6 +461,41 @@ class PackageValueSetProvider extends AbstractValueSetProvider { return this.#excludedUrls.size === 0 ? urls : urls.filter(url => !this.#excludedUrls.has(url)); } + /** + * The url -> versions of this package's value sets, when it is an hl7.terminology package + * (see AbstractValueSetProvider.thoVersions) + * + * @param {Map>} thoVersions - added to + */ + thoVersions(thoVersions) { + if (!isTHOPackage(this.sourcePackage())) { + return; + } + for (const vs of this.valueSetMap.values()) { + if (!vs || !vs.url) { + continue; + } + if (!thoVersions.has(vs.url)) { + thoVersions.set(vs.url, new Set()); + } + if (vs.version) { + thoVersions.get(vs.url).add(vs.version); + } + } + } + + /** + * When this is a FHIR core package, keep only the pinned references to value sets THO also + * provides (see AbstractValueSetProvider.yieldToTHO and keepPinnedOnly) + * + * @param {Map>} thoVersions - url -> the versions hl7.terminology provides + */ + yieldToTHO(thoVersions) { + if (VersionUtilities.isCorePackage(this.sourcePackage())) { + this.keepPinnedOnly(thoVersions); + } + } + // the value sets left out by keepPinnedOnly - by url, and by their own (unprefixed) id #excludedUrls = new Set(); #excludedIds = new Set(); From fd1a7fb8e23c7432c3cc6d0e33b627b13c33874c Mon Sep 17 00:00:00 2001 From: Josh Mandel Date: Mon, 2 Nov 2026 09:25:32 -0600 Subject: [PATCH 06/12] cs-cs: let a CodeSystem object choose its provider (providerFactory) FhirCodeSystemProvider.create picks a provider class from a registry keyed by the CodeSystem's url (providerClassFor: ICF today), so it can't pick one by where the content is kept. A CodeSystem object may now have a providerFactory(opContext, supplements, cls) method, asked before the registry and told the class the registry would use; the provider it returns is used, and null leaves it to the registry. A package CodeSystem kept in a database can then get a provider that reads it there, while version, THO-precedence and supplement handling stay as they are. CodeSystems without the method, which is all of main's, get the same provider as before, ICF included. Co-Authored-By: Claude Opus 5.5 --- tests/cs/cs-cs-provider-factory.test.js | 60 +++++++++++++++++++++++++ tx/cs/cs-cs.js | 14 ++++++ 2 files changed, 74 insertions(+) create mode 100644 tests/cs/cs-cs-provider-factory.test.js diff --git a/tests/cs/cs-cs-provider-factory.test.js b/tests/cs/cs-cs-provider-factory.test.js new file mode 100644 index 00000000..47aafa56 --- /dev/null +++ b/tests/cs/cs-cs-provider-factory.test.js @@ -0,0 +1,60 @@ +/** + * FhirCodeSystemProvider.create: a CodeSystem object may choose its provider (providerFactory) + * before the url registry does. Without one - every CodeSystem main loads - the registry decides + * as before, ICF included. + */ +const fs = require('fs'); +const path = require('path'); +const {CodeSystem} = require('../../tx/library/codesystem'); +const {FhirCodeSystemProvider} = require('../../tx/cs/cs-cs'); +const {IcfCodeSystemProvider} = require('../../tx/cs/cs-icf'); +const {OperationContext} = require('../../tx/operation-context'); +const {TestUtilities} = require('../test-utilities'); + +const ICF_JSON = path.join(__dirname, '../data/cs-icf-test.json'); +const plainJson = () => ({resourceType: 'CodeSystem', url: 'http://example.org/cs', status: 'active', content: 'complete', concept: [{code: 'a'}]}); + +/** a stored CodeSystem that builds its own provider for plain CodeSystems, and leaves registered subclasses alone */ +class StoredCodeSystem extends CodeSystem { + providerFactory(opContext, supplements, cls) { + this.asked = (this.asked || 0) + 1; + if (cls !== FhirCodeSystemProvider) { + return null; + } + return new StoredProvider(opContext, this, supplements); + } +} +class StoredProvider extends FhirCodeSystemProvider { } + +describe('FhirCodeSystemProvider.create and CodeSystem.providerFactory', () => { + let opContext; + + beforeAll(async () => { + const i18n = await TestUtilities.loadTranslations(await TestUtilities.loadLanguageDefinitions()); + opContext = new OperationContext('en-US', i18n); + }); + + test('no providerFactory: the url registry decides, as before', () => { + expect(FhirCodeSystemProvider.create(opContext, new CodeSystem(plainJson()), []).constructor).toBe(FhirCodeSystemProvider); + const icf = new CodeSystem(JSON.parse(fs.readFileSync(ICF_JSON, 'utf8'))); + expect(FhirCodeSystemProvider.create(opContext, icf, [])).toBeInstanceOf(IcfCodeSystemProvider); + }); + + test('a providerFactory supplies the provider, and is told the registry\'s choice', () => { + const cs = new StoredCodeSystem(plainJson()); + const supplements = []; + const spy = jest.spyOn(cs, 'providerFactory'); + const p = FhirCodeSystemProvider.create(opContext, cs, supplements); + expect(p).toBeInstanceOf(StoredProvider); + expect(spy).toHaveBeenCalledWith(opContext, supplements, FhirCodeSystemProvider); + expect(p.codeSystem).toBe(cs); + }); + + test('a providerFactory that returns null leaves it to the registry (ICF stays ICF)', () => { + const icf = new StoredCodeSystem(JSON.parse(fs.readFileSync(ICF_JSON, 'utf8'))); + const p = FhirCodeSystemProvider.create(opContext, icf, []); + expect(icf.asked).toBe(1); + expect(p).toBeInstanceOf(IcfCodeSystemProvider); + expect(p).not.toBeInstanceOf(StoredProvider); + }); +}); diff --git a/tx/cs/cs-cs.js b/tx/cs/cs-cs.js index 37b14bfb..812dd1a4 100644 --- a/tx/cs/cs-cs.js +++ b/tx/cs/cs-cs.js @@ -200,6 +200,14 @@ class FhirCodeSystemProvider extends BaseCSServices { /** * The provider for a CodeSystem resource: a FhirCodeSystemProvider, or the subclass * registered for its url. Always use this rather than the constructor. + * + * A CodeSystem object may choose its own provider: when it has a providerFactory method, that is + * asked first - providerFactory(opContext, supplements, cls), where cls is the class the url + * registry would use - and the provider it returns is used. If it returns null, or the object + * has no such method (as for every CodeSystem main loads), the registry decides, as before. A + * CodeSystem whose content lives in a database, say, can supply a provider that reads it there; + * a factory that only handles plain CodeSystems returns null when cls is a registered subclass. + * * @param {OperationContext} opContext * @param {CodeSystem} codeSystem * @param {CodeSystem[]} supplements @@ -207,6 +215,12 @@ class FhirCodeSystemProvider extends BaseCSServices { */ static create(opContext, codeSystem, supplements) { const cls = providerClassFor(codeSystem.url); + if (typeof codeSystem.providerFactory === 'function') { + const provider = codeSystem.providerFactory(opContext, supplements, cls); + if (provider) { + return provider; + } + } return new cls(opContext, codeSystem, supplements); } From 7fd0469ae780f0d7bf3e3599cb88a9489936c812 Mon Sep 17 00:00:00 2001 From: Josh Mandel Date: Mon, 2 Nov 2026 12:12:15 -0600 Subject: [PATCH 07/12] expand: a handed-over whole-system include over the limit is refused naming url|version checkSource's up-front too-costly refusal of a whole-system include is skipped for a compose handed to the provider, so an over-limit selection was refused later by includeCode, which names the plain url (tx-ecosystem loinc/loinc-expand-all expects url|version). processCodes now applies the same condition and message once the provider has reported the selection's total. Co-Authored-By: Claude Opus 5.5 --- tests/tx/expand-process-selection.test.js | 35 +++++++++++++++++++++-- tx/workers/expand.js | 9 ++++++ 2 files changed, 42 insertions(+), 2 deletions(-) diff --git a/tests/tx/expand-process-selection.test.js b/tests/tx/expand-process-selection.test.js index cb28b269..c1711787 100644 --- a/tests/tx/expand-process-selection.test.js +++ b/tests/tx/expand-process-selection.test.js @@ -64,7 +64,7 @@ function select(includes, excludes) { describe('$expand: handing a whole compose to the provider (processSelection)', () => { let app; let calls; - let behaviour; // 'handle' | 'decline' | 'no-total' + let behaviour; // 'handle' | 'decline' | 'no-total' | 'large' | 'off' const spies = []; beforeAll(async () => { @@ -85,7 +85,8 @@ describe('$expand: handing a whole compose to the provider (processSelection)', for (const code of page) { set.add(this.codeSystem.getConceptByCode(code)); } - return { sets: [set], total: behaviour === 'no-total' ? -1 : all.length }; + const total = behaviour === 'no-total' ? -1 : behaviour === 'large' ? 1000000 : all.length; + return { sets: [set], total }; })); }, 60000); @@ -160,6 +161,36 @@ describe('$expand: handing a whole compose to the provider (processSelection)', expect(handled.expansion.total).toEqual(usual.expansion.total); }); + test('a whole-system selection over the limit is refused as on the usual path, naming url|version', async () => { + const refuse = async (vs) => { + const res = await request(app).post('/tx/r5/ValueSet/$expand') + .set('Content-Type', 'application/json') + .send({ resourceType: 'Parameters', parameter: [ + { name: 'tx-resource', resource: CS }, { name: 'valueSet', resource: vs }, { name: 'limit', valueInteger: 5 } + ] }); + expect(res.status).toBe(422); + expect(res.body.issue[0].code).toBe('too-costly'); + return res.body.issue[0]; + }; + const vs = valueSet({ include: [wholeSystem] }); + behaviour = 'large'; // the provider reports a total far over the limit + const handled = await refuse(vs); + expect(calls).toHaveLength(1); + behaviour = 'off'; + const usual = await refuse(vs); + expect(handled.details.text).toEqual(usual.details.text); + expect(handled.details.text).toContain(vs.url + '|1.0.0'); + expect(handled.details.text).toContain('>5'); + }); + + test('a page that fits inside the limit is not refused', async () => { + behaviour = 'large'; + const vs = await expand(app, valueSet({ include: [wholeSystem] }), + [{ name: 'limit', valueInteger: 5 }, { name: 'count', valueInteger: 3 }]); + expect(codesOf(vs)).toEqual(codes.slice(0, 3)); + expect(vs.expansion.total).toBe(1000000); + }); + test.each([ ['a text filter', { include: [wholeSystem] }, [{ name: 'filter', valueString: 'c03' }]], ['an enumerated concept list', { include: [{ system: CS_URL, concept: [{ code: 'c03' }, { code: 'c04' }] }] }, []], diff --git a/tx/workers/expand.js b/tx/workers/expand.js index a18a0f68..eed6acc4 100644 --- a/tx/workers/expand.js +++ b/tx/workers/expand.js @@ -773,6 +773,15 @@ class ValueSetExpander { return false; } this.worker.opContext.log('compose handled by the code system provider'); + // checkSource's up-front refusal of a whole-system include is skipped for a compose handed to + // the provider (it is told the paging). Apply it here, now that the selection's total is known, + // with the same condition and message: it names the versioned url, where includeCode's refusal + // would name the plain url + const wholeSystemInclude = includes.some(cset => !(cset.filter || []).length); + if (wholeSystemInclude && this.count !== 0 && this.limitCount > 0 && selection.total > this.limitCount + && !(this.count > 0 && this.count + Math.max(this.offset, 0) <= this.limitCount)) { + throw new Issue("error", "too-costly", null, 'VALUESET_TOO_COSTLY', this.worker.i18n.translate('VALUESET_TOO_COSTLY', this.params.httpLanguages, [vsSrc.vurl || vsSrc.url, '>' + this.limitCount]), null, 422).withDiagnostics(this.worker.opContext.diagnostics()); + } for (const cset of [...includes, ...excludes]) { this.worker.checkSupplements(cs, cset, this.requiredSupplements, this.usedSupplements); From 120521f40ce134c816c6566a7e42a1b4c517c8ab Mon Sep 17 00:00:00 2001 From: Josh Mandel Date: Mon, 2 Nov 2026 12:12:58 -0600 Subject: [PATCH 08/12] cs-api processSelection: an optional context naming the value set; providers may refuse as too costly processSelection gets an optional trailing argument, context = { valueSet: { url, version } }, the value set whose compose it is handed, so a provider that refuses can name it without reading the request's state. The default is {} and a provider that ignores the argument behaves as before. cs-api.js and cs-api.md document that a provider may throw the standard too-costly Issue, with the exact form expand.js uses, and (cs-api.md) the over-the-limit refusal processCodes applies to a whole-system include. Co-Authored-By: Claude Opus 5.5 --- tests/tx/expand-process-selection.test.js | 36 +++++++++++++++++++++-- tx/cs/cs-api.js | 14 ++++++++- tx/cs/cs-api.md | 21 ++++++++++++- tx/workers/expand.js | 3 +- 4 files changed, 68 insertions(+), 6 deletions(-) diff --git a/tests/tx/expand-process-selection.test.js b/tests/tx/expand-process-selection.test.js index c1711787..b5cd6f17 100644 --- a/tests/tx/expand-process-selection.test.js +++ b/tests/tx/expand-process-selection.test.js @@ -11,6 +11,7 @@ const request = require('supertest'); const { getTestApp, shutdownTestApp } = require('./setup'); const { FhirCodeSystemProvider, FhirCodeSystemProviderFilterContext } = require('../../tx/cs/cs-cs'); +const { Issue } = require('../../tx/library/operation-outcome'); const CS_URL = 'http://example.org/fhir/CodeSystem/process-selection-cs'; const N = 12; @@ -64,7 +65,7 @@ function select(includes, excludes) { describe('$expand: handing a whole compose to the provider (processSelection)', () => { let app; let calls; - let behaviour; // 'handle' | 'decline' | 'no-total' | 'large' | 'off' + let behaviour; // 'handle' | 'decline' | 'no-total' | 'large' | 'refuse' | 'off' const spies = []; beforeAll(async () => { @@ -73,11 +74,19 @@ describe('$expand: handing a whole compose to the provider (processSelection)', spies.push(jest.spyOn(FhirCodeSystemProvider.prototype, 'handlesSelecting').mockImplementation(function () { return this.system() === CS_URL && behaviour !== 'off' ? true : origHandles.call(this); })); - spies.push(jest.spyOn(FhirCodeSystemProvider.prototype, 'processSelection').mockImplementation(async function (params, includes, excludes, excludeInactive, offset, count) { - calls.push({ includes, excludes, excludeInactive, offset, count }); + spies.push(jest.spyOn(FhirCodeSystemProvider.prototype, 'processSelection').mockImplementation(async function (params, includes, excludes, excludeInactive, offset, count, context) { + calls.push({ includes, excludes, excludeInactive, offset, count, context }); if (behaviour === 'decline') { return null; } + if (behaviour === 'refuse') { + // the form cs-api.js documents for a provider that won't produce the selection + const vs = context.valueSet; + const vurl = vs.version ? vs.url + '|' + vs.version : vs.url; + throw new Issue('error', 'too-costly', null, 'VALUESET_TOO_COSTLY', + this.opContext.i18n.translate('VALUESET_TOO_COSTLY', params.httpLanguages, [vurl, '>' + 7]), + null, 422).withDiagnostics(this.opContext.diagnostics()); + } const all = select(includes, excludes); const start = Math.max(offset, 0); const page = count < 0 ? all.slice(start) : all.slice(start, start + count); @@ -183,6 +192,27 @@ describe('$expand: handing a whole compose to the provider (processSelection)', expect(handled.details.text).toContain('>5'); }); + test('the provider is told which value set the compose belongs to', async () => { + const vs = valueSet({ include: [wholeSystem] }); + await expand(app, vs, [{ name: 'count', valueInteger: 2 }]); + expect(calls).toHaveLength(1); + expect(calls[0].context).toEqual({ valueSet: { url: vs.url, version: '1.0.0' } }); + }); + + test('a provider may refuse as too costly with the standard Issue', async () => { + behaviour = 'refuse'; + const vs = valueSet({ include: [wholeSystem] }); + const res = await request(app).post('/tx/r5/ValueSet/$expand') + .set('Content-Type', 'application/json') + .send({ resourceType: 'Parameters', parameter: [ + { name: 'tx-resource', resource: CS }, { name: 'valueSet', resource: vs } + ] }); + expect(res.status).toBe(422); + expect(res.body.issue[0].code).toBe('too-costly'); + expect(res.body.issue[0].details.text).toBe( + "The value set '" + vs.url + "|1.0.0' expansion has too many codes to produce (>7)"); + }); + test('a page that fits inside the limit is not refused', async () => { behaviour = 'large'; const vs = await expand(app, valueSet({ include: [wholeSystem] }), diff --git a/tx/cs/cs-api.js b/tx/cs/cs-api.js index 5cd2ca8b..16bfb40a 100644 --- a/tx/cs/cs-api.js +++ b/tx/cs/cs-api.js @@ -684,6 +684,18 @@ class CodeSystemProvider { * @param {boolean} excludeInactive: whether the value set leaves out inactive codes (compose.inactive = false) * @param {int} offset the index of the first code to return, -1 when the request gave no offset (start at 0) * @param {int} count the most codes to return, -1 when the request gave no count (all of them); 0 asks for the total only + * @param {Object} [context] what the selection is for. `context.valueSet` is `{ url, version }` of the + * value set whose compose this is (either may be undefined for a value set given inline without + * them). A provider may ignore it. + * + * A provider that won't produce a selection because it would cost too much may throw the standard + * too-costly Issue, in the form expand.js uses when it refuses a value set (Issue from + * tx/library/operation-outcome.js; vurl is url|version, or url when there is no version): + * + * throw new Issue('error', 'too-costly', null, 'VALUESET_TOO_COSTLY', + * this.opContext.i18n.translate('VALUESET_TOO_COSTLY', params.httpLanguages, [vurl, '>' + limit]), + * null, 422).withDiagnostics(this.opContext.diagnostics()); + * * @returns {Promise<{sets: FilterConceptSet[], total: number, notClosed?: boolean}|null>} null to decline. * Otherwise `sets` (in general one; iterated with filterMore/filterConcept) hold the page, * `total` is the number of codes in the whole selection before paging, or -1 when the @@ -691,7 +703,7 @@ class CodeSystemProvider { * when the selection is open-ended (a grammar-based code system), so the expansion is * marked unclosed */ - processSelection(params, includes, excludes, excludeInactive, offset, count) { + processSelection(params, includes, excludes, excludeInactive, offset, count, context = {}) { // well, you only need to override if handlesSelecting=true, but that's the only time this will be called throw new Error("Must override"); } diff --git a/tx/cs/cs-api.md b/tx/cs/cs-api.md index f4cb1c0b..c0a9afcb 100644 --- a/tx/cs/cs-api.md +++ b/tx/cs/cs-api.md @@ -191,7 +191,7 @@ hand the entire include/exclude list to the provider in one call rather than usi workflow above. Override `handlesSelecting()` to return `true` and then implement -`processSelection(params, includes, excludes, excludeInactive, offset, count)`. +`processSelection(params, includes, excludes, excludeInactive, offset, count, context)`. - **When it is called.** `$expand` hands over a compose that selects from this code system alone (one system|version), imports no value sets, has no text filter and lists no concepts. @@ -206,6 +206,25 @@ Override `handlesSelecting()` to return `true` and then implement before paging, or -1 if the provider hasn't counted it (the expansion then has no total); `notClosed` marks an open-ended selection (a grammar-based code system). +- **Context.** The optional last argument, `context`, says what the selection is for: + `context.valueSet` is `{ url, version }` of the value set whose compose this is (either may be + undefined for a value set given inline without them). A provider may ignore it. +- **Too costly.** A provider that won't produce a selection because it would cost too much may + throw the standard too-costly `Issue` (tx/library/operation-outcome.js), in the form `$expand` + uses when it refuses a value set. Here `vurl` is `url|version`, or `url` when there is no + version: + + ```js + throw new Issue('error', 'too-costly', null, 'VALUESET_TOO_COSTLY', + this.opContext.i18n.translate('VALUESET_TOO_COSTLY', params.httpLanguages, [vurl, '>' + limit]), + null, 422).withDiagnostics(this.opContext.diagnostics()); + ``` + + The response is a 422 with a `too-costly` issue, as for a value set `$expand` refuses itself. +- **Over the limit.** When an include takes the whole system and the reported `total` is over the + request's limit, `$expand` refuses as it does on the usual path, unless the requested page + fits inside the limit. + The default `handlesSelecting()` returns `false`, so a provider that doesn't override it is never asked. diff --git a/tx/workers/expand.js b/tx/workers/expand.js index eed6acc4..799e9eaf 100644 --- a/tx/workers/expand.js +++ b/tx/workers/expand.js @@ -768,7 +768,8 @@ class ValueSetExpander { } } - const selection = await cs.processSelection(this.params, includes, excludes, excludeInactive, this.offset, this.count); + const selection = await cs.processSelection(this.params, includes, excludes, excludeInactive, this.offset, this.count, + { valueSet: { url: vsSrc.url, version: vsSrc.version } }); if (selection == null) { return false; } From c1a8cff819bdd7f0c0b6fa556c1db86e1e0ea795 Mon Sep 17 00:00:00 2001 From: Josh Mandel Date: Mon, 2 Nov 2026 12:13:10 -0600 Subject: [PATCH 09/12] cs-cs: export providerClassFor A specialised provider class (ICF) must be known when the server loads, so that code storing package CodeSystems elsewhere (a database, say) can leave those to the url registry. Exported unchanged; a test pins what it returns. Co-Authored-By: Claude Opus 5.5 --- tests/cs/cs-cs-provider-factory.test.js | 7 ++++++- tx/cs/cs-cs.js | 4 +++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/tests/cs/cs-cs-provider-factory.test.js b/tests/cs/cs-cs-provider-factory.test.js index 47aafa56..f0c2e8d6 100644 --- a/tests/cs/cs-cs-provider-factory.test.js +++ b/tests/cs/cs-cs-provider-factory.test.js @@ -6,7 +6,7 @@ const fs = require('fs'); const path = require('path'); const {CodeSystem} = require('../../tx/library/codesystem'); -const {FhirCodeSystemProvider} = require('../../tx/cs/cs-cs'); +const {FhirCodeSystemProvider, providerClassFor} = require('../../tx/cs/cs-cs'); const {IcfCodeSystemProvider} = require('../../tx/cs/cs-icf'); const {OperationContext} = require('../../tx/operation-context'); const {TestUtilities} = require('../test-utilities'); @@ -34,6 +34,11 @@ describe('FhirCodeSystemProvider.create and CodeSystem.providerFactory', () => { opContext = new OperationContext('en-US', i18n); }); + test('providerClassFor is exported: ICF has its own class, other urls the plain one', () => { + expect(providerClassFor('http://hl7.org/fhir/sid/icf')).toBe(IcfCodeSystemProvider); + expect(providerClassFor('http://example.org/cs')).toBe(FhirCodeSystemProvider); + }); + test('no providerFactory: the url registry decides, as before', () => { expect(FhirCodeSystemProvider.create(opContext, new CodeSystem(plainJson()), []).constructor).toBe(FhirCodeSystemProvider); const icf = new CodeSystem(JSON.parse(fs.readFileSync(ICF_JSON, 'utf8'))); diff --git a/tx/cs/cs-cs.js b/tx/cs/cs-cs.js index 812dd1a4..6c5c6afa 100644 --- a/tx/cs/cs-cs.js +++ b/tx/cs/cs-cs.js @@ -184,7 +184,8 @@ class FhirCodeSystemProviderFilterContext { // Code systems loaded as ordinary CodeSystem resources (from a package, or supplied by a // client) that need behaviour beyond the resource: a grammar, say. Keyed on the CodeSystem // url; the class extends FhirCodeSystemProvider, so the content still comes from the -// resource. Required lazily, because the subclasses require this module. +// resource. Required lazily, because the subclasses require this module. Exported, so code that +// stores CodeSystems when the server loads can tell which ones need their own class. let specialisedProviders = null; function providerClassFor(url) { @@ -1926,6 +1927,7 @@ class FhirCodeSystemFactory extends CodeSystemFactoryProvider { } module.exports = { + providerClassFor, FhirCodeSystemFactory, FhirCodeSystemProvider, FhirCodeSystemProviderContext, From f7fcd3139f9338d08c630d4adfc29e0ec9fa19d6 Mon Sep 17 00:00:00 2001 From: Josh Mandel Date: Mon, 2 Nov 2026 13:14:51 -0600 Subject: [PATCH 10/12] expand: for a compose the provider pages, only the page counts against the limit The whole-system too-costly check applied in processCodes copied the usual path's condition (offset + count must fit the limit). That condition exists because the usual path builds every code before the page; a provider that pages itself does not, so a deep page (offset 200,000, count 10) was refused although filtered selections page freely. Only count is checked now; a request with no count, or a count over the limit, is still refused as before. Co-Authored-By: Claude Opus 5.5 --- tests/tx/expand-process-selection.test.js | 8 ++++++++ tx/workers/expand.js | 8 +++++--- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/tests/tx/expand-process-selection.test.js b/tests/tx/expand-process-selection.test.js index b5cd6f17..91e16868 100644 --- a/tests/tx/expand-process-selection.test.js +++ b/tests/tx/expand-process-selection.test.js @@ -221,6 +221,14 @@ describe('$expand: handing a whole compose to the provider (processSelection)', expect(vs.expansion.total).toBe(1000000); }); + test('a deep page is not refused: only the page counts against the limit', async () => { + behaviour = 'large'; + const vs = await expand(app, valueSet({ include: [wholeSystem] }), + [{ name: 'limit', valueInteger: 5 }, { name: 'offset', valueInteger: 4 }, { name: 'count', valueInteger: 3 }]); + expect(codesOf(vs)).toEqual(codes.slice(4, 7)); + expect(vs.expansion.total).toBe(1000000); + }); + test.each([ ['a text filter', { include: [wholeSystem] }, [{ name: 'filter', valueString: 'c03' }]], ['an enumerated concept list', { include: [{ system: CS_URL, concept: [{ code: 'c03' }, { code: 'c04' }] }] }, []], diff --git a/tx/workers/expand.js b/tx/workers/expand.js index 799e9eaf..2f61a208 100644 --- a/tx/workers/expand.js +++ b/tx/workers/expand.js @@ -776,11 +776,13 @@ class ValueSetExpander { this.worker.opContext.log('compose handled by the code system provider'); // checkSource's up-front refusal of a whole-system include is skipped for a compose handed to // the provider (it is told the paging). Apply it here, now that the selection's total is known, - // with the same condition and message: it names the versioned url, where includeCode's refusal - // would name the plain url + // with the same message: it names the versioned url, where includeCode's refusal would name the + // plain url. Only the page counts against the limit: the provider pages itself, so a deep page + // costs no more than the first one (on the usual path offset + count must fit, because the + // codes before the page are built too) const wholeSystemInclude = includes.some(cset => !(cset.filter || []).length); if (wholeSystemInclude && this.count !== 0 && this.limitCount > 0 && selection.total > this.limitCount - && !(this.count > 0 && this.count + Math.max(this.offset, 0) <= this.limitCount)) { + && !(this.count > 0 && this.count <= this.limitCount)) { throw new Issue("error", "too-costly", null, 'VALUESET_TOO_COSTLY', this.worker.i18n.translate('VALUESET_TOO_COSTLY', this.params.httpLanguages, [vsSrc.vurl || vsSrc.url, '>' + this.limitCount]), null, 422).withDiagnostics(this.worker.opContext.diagnostics()); } From 1a66d65cafccfa39cec227cdd59bea8c2384bba2 Mon Sep 17 00:00:00 2001 From: Josh Mandel Date: Tue, 3 Nov 2026 06:58:31 -0600 Subject: [PATCH 11/12] tests: processSelection reports requested properties, and leaves supplement-property filters to the usual path Co-Authored-By: Claude Opus 5.5 --- tests/tx/expand-process-selection.test.js | 30 ++++++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-) diff --git a/tests/tx/expand-process-selection.test.js b/tests/tx/expand-process-selection.test.js index 91e16868..b87037ae 100644 --- a/tests/tx/expand-process-selection.test.js +++ b/tests/tx/expand-process-selection.test.js @@ -18,7 +18,15 @@ const N = 12; const codes = Array.from({ length: N }, (_, i) => 'c' + String(i).padStart(2, '0')); const CS = { resourceType: 'CodeSystem', url: CS_URL, version: '1.0.0', status: 'active', content: 'complete', - concept: codes.map(code => ({ code, display: 'Display ' + code })) + property: [{ code: 'grade', type: 'code' }], + concept: codes.map((code, i) => ({ code, display: 'Display ' + code, property: [{ code: 'grade', valueCode: 'g' + (i % 3) }] })) +}; +// a supplement that gives two of the codes a property the code system doesn't have +const SUPP_URL = 'http://example.org/fhir/CodeSystem/process-selection-supplement'; +const SUPPLEMENT = { + resourceType: 'CodeSystem', url: SUPP_URL, version: '1', status: 'active', content: 'supplement', supplements: CS_URL, + property: [{ code: 'group', type: 'code' }], + concept: [{ code: 'c03', property: [{ code: 'group', valueCode: 'x' }] }, { code: 'c04', property: [{ code: 'group', valueCode: 'x' }] }] }; let vsCounter = 0; @@ -229,6 +237,26 @@ describe('$expand: handing a whole compose to the provider (processSelection)', expect(vs.expansion.total).toBe(1000000); }); + test('the requested properties are on each code the provider selected', async () => { + const vs = await expand(app, valueSet({ include: [wholeSystem] }), + [{ name: 'property', valueString: 'grade' }, { name: 'count', valueInteger: 4 }]); + expect(calls).toHaveLength(1); + expect(vs.expansion.contains.map(c => (c.property || []).map(p => p.code + '=' + p.valueCode).join())).toEqual(['grade=g0', 'grade=g1', 'grade=g2', 'grade=g0']); + }); + + test('a filter on a property a supplement defines: not handed over, evaluated from the supplement', async () => { + const vs = valueSet({ include: [{ system: CS_URL, filter: [{ property: 'group', op: '=', value: 'x' }] }] }); + vs.extension = [{ url: 'http://hl7.org/fhir/StructureDefinition/valueset-supplement', valueCanonical: SUPP_URL + '|1' }]; + const res = await request(app).post('/tx/r5/ValueSet/$expand') + .set('Content-Type', 'application/json') + .send({ resourceType: 'Parameters', parameter: [ + { name: 'tx-resource', resource: CS }, { name: 'tx-resource', resource: SUPPLEMENT }, { name: 'valueSet', resource: vs } + ] }); + expect(res.status).toBe(200); + expect(calls).toHaveLength(0); + expect(codesOf(res.body)).toEqual(['c03', 'c04']); + }); + test.each([ ['a text filter', { include: [wholeSystem] }, [{ name: 'filter', valueString: 'c03' }]], ['an enumerated concept list', { include: [{ system: CS_URL, concept: [{ code: 'c03' }, { code: 'c04' }] }] }, []], From 9aac1d273b9f021afc4a066b8ab97a30135bda4b Mon Sep 17 00:00:00 2001 From: Josh Mandel Date: Tue, 3 Nov 2026 20:45:36 -0600 Subject: [PATCH 12/12] cs-api ancestorsOf: $closure relates concepts from ancestor sets where a provider can list them Adding k concepts to a closure table of n ran subsumesTest k*n times. For LOINC a test is 2-4 SQLite statements (~140 us), so 200 concepts into a table of 400 took 13.8 s, and the 20 s deadline capped a call at about 140k pairs. CodeSystemProvider gets an optional ancestorsOf(code): a Set of the codes a code sits under, or null. The default is null. When a provider gives a set for both codes of a pair, the closure worker reads the relationship from the two sets (the same outcome subsumesTest gives, which the contract requires); otherwise it calls subsumesTest for that pair, as before. Each set is asked for once per call, when first needed, and not kept: no schema change, and never stale when a code system changes. No provider here implements it, so every table is built as before. Tests: a fake provider over random multi-parent hierarchies; with the hook, for some codes only, and without it, every response and the resync are the same, and the hook needs no subsumesTest calls. Co-Authored-By: Claude Opus 5.5 --- tests/tx/closure-ancestors.test.js | 220 +++++++++++++++++++++++++++++ tx/cs/cs-api.js | 21 +++ tx/cs/cs-api.md | 1 + tx/workers/closure.js | 26 +++- 4 files changed, 266 insertions(+), 2 deletions(-) create mode 100644 tests/tx/closure-ancestors.test.js diff --git a/tests/tx/closure-ancestors.test.js b/tests/tx/closure-ancestors.test.js new file mode 100644 index 00000000..fa4ed839 --- /dev/null +++ b/tests/tx/closure-ancestors.test.js @@ -0,0 +1,220 @@ +/** + * $closure relates concepts from ancestor sets when the provider can list them + * (CodeSystemProvider.ancestorsOf), and by subsumesTest per pair when it can't. + * + * The provider is a fake over a random hierarchy (several parents per code, so some codes + * sit under others by more than one path). The same random sequence of adds is run three + * ways - provider without the hook, with it, and with it for some codes only - and every + * response, and the table as a resync from 0 reads it, must be the same; the hook must + * need far fewer provider calls. + */ + +const ClosureWorker = require('../../tx/workers/closure'); +const { ClosureStore } = require('../../tx/closure/closure-store'); +const { OperationContext } = require('../../tx/operation-context'); +const { CodeSystemProvider } = require('../../tx/cs/cs-api'); +const { TestUtilities } = require('../test-utilities'); + +const SYS = 'http://example.org/fhir/CodeSystem/closure-ancestors'; +const OTHER = 'http://example.org/fhir/CodeSystem/closure-ancestors-other'; + +/** a seeded random number generator (mulberry32), so a failure can be reproduced */ +function rng(seed) { + let a = seed >>> 0; + return () => { + a = (a + 0x6D2B79F5) >>> 0; + let t = a; + t = Math.imul(t ^ (t >>> 15), t | 1); + t ^= t + Math.imul(t ^ (t >>> 7), t | 61); + return ((t ^ (t >>> 14)) >>> 0) / 4294967296; + }; +} + +/** codes c0..c(size-1); each code after the first few has 1-3 parents among the codes before it */ +function hierarchy(size, random) { + const parents = new Map(); + for (let i = 0; i < size; i++) { + const ps = new Set(); + if (i >= 3) { + const count = 1 + Math.floor(random() * 3); + for (let j = 0; j < count; j++) { + ps.add('c' + Math.floor(random() * i)); + } + } + parents.set('c' + i, [...ps]); + } + const ancestors = new Map(); + const walk = code => { + if (!ancestors.has(code)) { + const set = new Set(); + for (const p of parents.get(code)) { + set.add(p); + for (const a of walk(p)) { + set.add(a); + } + } + ancestors.set(code, set); + } + return ancestors.get(code); + }; + for (const code of parents.keys()) { + walk(code); + } + return ancestors; +} + +/** + * hook: 'none' (the default ancestorsOf), 'all', or 'some' (null for codes whose number is a + * multiple of 3, as a provider does for a code it can't answer for) + */ +class FakeProvider extends CodeSystemProvider { + constructor(opContext, system, ancestors, hook, calls) { + super(opContext, []); + this.sys = system; + this.anc = ancestors; + this.hook = hook; + this.calls = calls; + } + system() { return this.sys; } + version() { return '1'; } + async locate(code) { return this.anc.has(code) ? { context: { code }, message: null } : { context: null, message: 'unknown' }; } + async code(ctxt) { return ctxt.code; } + async display(ctxt) { return 'Display ' + ctxt.code; } + async subsumesTest(a, b) { + this.calls.subsumesTest++; + if (a === b) { + return 'equivalent'; + } + if (this.anc.get(b).has(a)) { + return 'subsumes'; + } + return this.anc.get(a).has(b) ? 'subsumed-by' : 'not-subsumed'; + } + async ancestorsOf(code) { + this.calls.ancestorsOf++; + if (this.hook === 'none' || (this.hook === 'some' && Number(code.substring(1)) % 3 === 0)) { + return super.ancestorsOf(code); + } + return new Set(this.anc.get(code)); + } +} + +function entriesOf(cm) { + const out = []; + for (const g of cm.group || []) { + for (const e of g.element || []) { + for (const t of e.target || []) { + out.push(`${g.source}|${e.code} > ${g.target}|${t.code} : ${t.relationship}${t.equivalence ? '/' + t.equivalence : ''}`); + } + } + } + return out; +} + +describe('$closure from ancestor sets (CodeSystemProvider.ancestorsOf)', () => { + let i18n; + let languages; + + beforeAll(async () => { + languages = await TestUtilities.loadLanguageDefinitions(); + i18n = await TestUtilities.loadTranslations(languages); + }); + + /** run the adds against a new table; returns every response's entries, the resync, and the calls */ + async function run(adds, ancestors, hook, fhirVersion = '5.0') { + const calls = { subsumesTest: 0, ancestorsOf: 0 }; + const store = new ClosureStore({ database: ':memory:' }); + store.open(); + try { + store.createTable('t', []); + const responses = []; + for (const codings of adds) { + const opContext = new OperationContext('en', i18n); + const worker = new ClosureWorker(opContext, { info() {}, debug() {}, warn() {}, error() {} }, null, languages, i18n, store, {}); + const providers = new Map([[SYS, new FakeProvider(opContext, SYS, ancestors, hook, calls)], + [OTHER, new FakeProvider(opContext, OTHER, ancestors, hook, calls)]]); + worker.setupAdditionalResources = () => {}; + worker.seeSourceProvider = () => {}; + worker.findCodeSystem = async system => providers.get(system); + const params = codings.map(c => ({ name: 'concept', valueCoding: c })); + const cm = await worker.add(store.getTable('t'), 't', params, fhirVersion); + responses.push({ version: cm.version, entries: entriesOf(cm) }); + } + const table = store.getTable('t'); + const resync = store.entriesSince(table.id, 0) + .map(e => `${e.source_system}|${e.source_code} > ${e.target_system}|${e.target_code} : ${e.relationship}`).sort(); + return { responses, resync, calls }; + } finally { + store.close(); + } + } + + /** a random sequence of adds: batches of 1-40 codes (some repeated, some from the other system) */ + function randomAdds(random, size, batches) { + const adds = []; + for (let b = 0; b < batches; b++) { + const k = 1 + Math.floor(random() * 40); + const codings = []; + for (let i = 0; i < k; i++) { + const system = random() < 0.15 ? OTHER : SYS; + codings.push({ system, code: 'c' + Math.floor(random() * size) }); + } + adds.push(codings); + } + return adds; + } + + for (const seed of [1, 2, 3, 4, 5]) { + test(`random tables (seed ${seed}): the same entries with and without the hook, with far fewer calls`, async () => { + const random = rng(seed); + const size = 150 + Math.floor(random() * 150); + const ancestors = hierarchy(size, random); + const adds = randomAdds(random, size, 8); + + const pairwise = await run(adds, ancestors, 'none'); + const hooked = await run(adds, ancestors, 'all'); + const some = await run(adds, ancestors, 'some'); + + expect(pairwise.resync.length).toBeGreaterThan(0); + expect(hooked.responses).toEqual(pairwise.responses); + expect(hooked.resync).toEqual(pairwise.resync); + expect(some.responses).toEqual(pairwise.responses); + expect(some.resync).toEqual(pairwise.resync); + + // without the hook: one ancestorsOf (answered null) per new concept, then every pair + expect(pairwise.calls.subsumesTest).toBeGreaterThan(1000); + // with it: no pairs at all, and at most one ancestor set per concept per call + expect(hooked.calls.subsumesTest).toBe(0); + const concepts = new Set(adds.flat().map(c => c.system + '|' + c.code)).size; + expect(hooked.calls.ancestorsOf).toBeLessThanOrEqual(concepts * adds.length); + expect(hooked.calls.ancestorsOf * 5).toBeLessThan(pairwise.calls.subsumesTest); + // some codes without a set: only the pairs that involve one are tested + expect(some.calls.subsumesTest).toBeGreaterThan(0); + expect(some.calls.subsumesTest).toBeLessThan(pairwise.calls.subsumesTest); + }); + } + + test('both directions, and the R4 equivalence, as subsumesTest gives them', async () => { + // c0 > c3 > c5, by hand + const ancestors = new Map([['c0', new Set()], ['c3', new Set(['c0'])], ['c5', new Set(['c3', 'c0'])], ['c7', new Set()]]); + const adds = [[{ system: SYS, code: 'c3' }], [{ system: SYS, code: 'c5' }, { system: SYS, code: 'c0' }, { system: SYS, code: 'c7' }]]; + for (const fhirVersion of ['5.0', '4.0']) { + const pairwise = await run(adds, ancestors, 'none', fhirVersion); + const hooked = await run(adds, ancestors, 'all', fhirVersion); + expect(hooked.responses).toEqual(pairwise.responses); + expect(hooked.resync).toEqual(pairwise.resync); + const legacy = fhirVersion === '4.0' ? '/subsumes' : ''; + expect(hooked.responses[1].entries).toEqual([ + `${SYS}|c3 > ${SYS}|c0 : source-is-narrower-than-target${legacy}`, + `${SYS}|c5 > ${SYS}|c0 : source-is-narrower-than-target${legacy}`, + `${SYS}|c5 > ${SYS}|c3 : source-is-narrower-than-target${legacy}` + ]); + } + }); + + test('the default ancestorsOf is null', async () => { + const opContext = new OperationContext('en', i18n); + const cs = new CodeSystemProvider(opContext, []); + expect(await cs.ancestorsOf('x')).toBeNull(); + }); +}); diff --git a/tx/cs/cs-api.js b/tx/cs/cs-api.js index f38bc6cc..ef2e18d9 100644 --- a/tx/cs/cs-api.js +++ b/tx/cs/cs-api.js @@ -772,6 +772,27 @@ class CodeSystemProvider { */ async subsumesTest(codeA, codeB) { return 'not-subsumed'; } + /** + * The codes a code sits under: every ancestor in the hierarchy subsumesTest() answers from, + * not just the parents, and not the code itself. Optional: $closure uses it, when it is given, + * to relate many concepts to one another from one call per concept instead of one + * subsumesTest() per pair. The default returns null (not supported), and $closure then calls + * subsumesTest() for each pair, as it always has. + * + * A provider that implements it must agree with its own subsumesTest(): for two different + * codes a and b that this method answers for, subsumesTest(a, b) is 'subsumes' exactly when a + * is in ancestorsOf(b), 'subsumed-by' exactly when b is in ancestorsOf(a) (and a is not in + * ancestorsOf(b)), and never 'equivalent'. The codes are given in the form code() returns them. + * For any code where that can't be promised - an unknown code, a code not in the form code() + * gives (so another code may be the same concept), a postcoordinated expression - return null, + * and the pairs that involve it are tested with subsumesTest(). + * + * @param {string | CodeSystemProviderContext} code + * @returns {Promise|null>} the ancestors' codes, or null when not supported for this code + */ + // eslint-disable-next-line no-unused-vars + async ancestorsOf(code) { return null; } + /** * @param {CodeSystemProviderContext} ctxt the context to add properties for diff --git a/tx/cs/cs-api.md b/tx/cs/cs-api.md index 8debe02b..9750663c 100644 --- a/tx/cs/cs-api.md +++ b/tx/cs/cs-api.md @@ -110,6 +110,7 @@ All are `async`. | `sameConcept(a, b)` | `false` | Returns `true` if `a` and `b` refer to the same concept (e.g. different expression forms). | | `isDisplay(designation)` | `false` | Called for designations not already marked with a standard display use code; return `true` to treat the designation as a display. | | `subsumesTest(codeA, codeB)` | `'not-subsumed'` | Returns one of `equivalent`, `subsumes`, `subsumed-by`, or `not-subsumed`. | +| `ancestorsOf(code)` | `null` | Optional. A `Set` of the codes this code sits under (all ancestors, not itself), in the form `code()` returns them. `$closure` uses it to relate concepts with one call per concept instead of one `subsumesTest()` per pair. It must agree with `subsumesTest()`: for different codes a and b it answers for, a is in `ancestorsOf(b)` exactly when `subsumesTest(a, b)` is `subsumes`, and the two are never `equivalent`. Return `null` for any code where that can't be promised (unknown, not in `code()` form, an expression); pairs involving it are tested with `subsumesTest()`. | | `extendLookup(ctxt, props, params)` | — | Called during `$lookup`; add any additional properties to `params`. | --- diff --git a/tx/workers/closure.js b/tx/workers/closure.js index b2ef1fb7..97be08a6 100644 --- a/tx/workers/closure.js +++ b/tx/workers/closure.js @@ -374,14 +374,36 @@ class ClosureWorker extends TerminologyWorker { this.limit('maxEntriesPerTable') - (fresh.length > 0 ? this.store.entryCount(table.id) : 0)); const entries = []; const out = []; - const before = existing.map(c => ({ ref: c.id, system: c.system, code: c.code, display: c.display })); + const before = existing.map(c => ({ key: c.system + '|' + c.code, ref: c.id, system: c.system, code: c.code, display: c.display })); + // Where the provider can list a code's ancestors (CodeSystemProvider.ancestorsOf), a pair + // is related from the two ancestor sets, which costs one provider call per concept rather + // than a subsumesTest per pair. Each set is asked for once per call, when first needed, + // and only for concepts of a system that has new concepts; a concept the provider has no + // set for (null, the default) is compared by subsumesTest, as before. The sets are not + // kept between calls, so they always come from the code system as it is now. + const ancestorSets = new Map(); // key -> Set of codes, or null + const ancestorsOf = async (cs, c) => { + if (!ancestorSets.has(c.key)) { + await this.checkAndYield('closure-ancestors'); + ancestorSets.set(c.key, await cs.ancestorsOf(c.code)); + } + return ancestorSets.get(c.key); + }; for (const n of fresh) { + const an = await ancestorsOf(n.cs, n); for (const o of before) { if (o.system !== n.system) { continue; } await this.checkAndYield('closure-subsumes'); - const outcome = await n.cs.subsumesTest(n.code, o.code); + const ao = an ? await ancestorsOf(n.cs, o) : null; + let outcome; + if (ao) { + // the same outcome subsumesTest gives (see ancestorsOf): n and o are different codes + outcome = ao.has(n.code) ? 'subsumes' : an.has(o.code) ? 'subsumed-by' : 'not-subsumed'; + } else { + outcome = await n.cs.subsumesTest(n.code, o.code); + } let source, target, relationship; if (outcome === 'subsumes') { // the new concept is the broader one source = o; target = n; relationship = NARROWER;