diff --git a/src/App.tsx b/src/App.tsx index 70f0cde4..a7d596db 100644 --- a/src/App.tsx +++ b/src/App.tsx @@ -86,6 +86,8 @@ function _createClientMapping({ const storageClassMapping: { [key: string]: number } = { default: 0 } const clientMapping: { [sopClassUID: string]: DicomWebManager } = {} + const defaultServers: ServerSettings[] = [] + settings.forEach((serverSettings) => { if (serverSettings.storageClasses != null) { serverSettings.storageClasses.forEach((sopClassUID) => { @@ -110,6 +112,7 @@ function _createClientMapping({ } storageClassMapping.default += 1 + defaultServers.push(serverSettings) clientMapping.default = new DicomWebManager({ baseUri, settings: [serverSettings], @@ -129,36 +132,35 @@ function _createClientMapping({ ) } - for (const key in storageClassMapping) { - if (key === 'default') { - continue - } - if (storageClassMapping[key] > 1) { - NotificationMiddleware.onError( - NotificationMiddlewareContext.SLIM, - new CustomError( - errorTypes.COMMUNICATION, - 'Only one configured server can specify a given storage class. ' + - `Storage class "${key}" is specified by more than one ` + - 'of the configured servers.', - ), - ) - } - } - + /** + * For each storage class explicitly assigned to a non-default server, wrap + * BOTH the default server and the specialty server(s) in the same manager. + * + * This makes derived data (SR/SEG/ANN/PM/PR) load from the primary store + * AND the secondary `gcp=` URL store at the same time (GH-320). Without + * this, specifying `gcp=` previously caused the default store to be + * skipped for those classes and SLIM only saw the secondary's derived data. + */ if (Object.keys(storageClassMapping).length > 1) { + const classToServers = new Map() settings.forEach((server) => { - const client = new DicomWebManager({ - baseUri, - settings: [server], - onError, - }) if (server.storageClasses != null) { server.storageClasses.forEach((sopClassUID) => { - clientMapping[sopClassUID] = client + const list = classToServers.get(sopClassUID) ?? [] + list.push(server) + classToServers.set(sopClassUID, list) }) } }) + + classToServers.forEach((specialtyServers, sopClassUID) => { + const combinedServers = [...defaultServers, ...specialtyServers] + clientMapping[sopClassUID] = new DicomWebManager({ + baseUri, + settings: combinedServers, + onError, + }) + }) } Object.values(StorageClasses).forEach((sopClassUID) => { diff --git a/src/DicomWebManager.ts b/src/DicomWebManager.ts index df65c6d8..34f0d614 100644 --- a/src/DicomWebManager.ts +++ b/src/DicomWebManager.ts @@ -24,6 +24,133 @@ interface Store { read: boolean write: boolean client: dwc.api.DICOMwebClient + settings: ServerSettings +} + +/** DICOM JSON tag keys used for cross-store deduplication of search results. */ +const STUDY_INSTANCE_UID_TAG = '0020000D' +const SERIES_INSTANCE_UID_TAG = '0020000E' +const SOP_INSTANCE_UID_TAG = '00080018' + +type DicomJsonObject = Record + +const getDicomTagValue = ( + obj: DicomJsonObject, + tag: string, +): string | undefined => { + const value = obj[tag]?.Value?.[0] + return typeof value === 'string' ? value : undefined +} + +/** + * Build a stable dedup key for a DICOM JSON search result. Falls back to a + * JSON-stringified representation when no UID is present so we never silently + * drop entries (e.g. malformed responses) but still merge actual duplicates. + */ +const buildDedupKey = ( + obj: DicomJsonObject, + tags: readonly string[], +): string => { + const parts: string[] = [] + for (const tag of tags) { + const v = getDicomTagValue(obj, tag) + if (v === undefined) { + return JSON.stringify(obj) + } + parts.push(v) + } + return parts.join('|') +} + +/** + * Run the same DICOMweb operation against every readable store in parallel, + * merge the results, and de-duplicate them by the supplied DICOM tag keys. + * + * Used to support reading derived data (SR/SEG/ANN/PM/PR) from BOTH the + * primary configured server and any secondary store specified via the URL + * `gcp` query parameter (see GH-320). + * + * Stores are queried independently. Per-store failures are logged but do not + * abort the merge: a missing/forbidden store on one side should not hide + * results that are available on the other. + */ +const searchAcrossStores = async ( + stores: Store[], + dedupTags: readonly string[], + call: (store: Store) => Promise, +): Promise => { + const readable = stores.filter((s) => s.read) + if (readable.length === 0) { + return [] + } + const results = await Promise.all( + readable.map(async (store) => { + try { + return await call(store) + } catch (error: unknown) { + if (process.env.NODE_ENV === 'development') { + console.warn( + `search against store "${store.id}" failed; ` + + 'continuing with the remaining stores', + error, + ) + } + return [] as T[] + } + }), + ) + + const merged: T[] = [] + const seen = new Set() + for (const items of results) { + for (const item of items) { + const key = buildDedupKey(item, dedupTags) + if (!seen.has(key)) { + seen.add(key) + merged.push(item) + } + } + } + return merged +} + +/** + * Try the same DICOMweb retrieve operation against each readable store in + * order, returning on the first success and falling through on failures + * (typically 404 when an instance only exists in a different store). + * + * Throws the last encountered error if every store fails so legitimate + * errors are still surfaced. + */ +const retrieveWithFallback = async ( + stores: Store[], + call: (store: Store) => Promise, +): Promise => { + const readable = stores.filter((s) => s.read) + if (readable.length === 0) { + throw new CustomError( + errorTypes.COMMUNICATION, + 'No readable DICOMweb store is configured.', + ) + } + let lastError: unknown + for (const store of readable) { + try { + return await call(store) + } catch (error: unknown) { + lastError = error + if (process.env.NODE_ENV === 'development') { + console.debug( + `retrieve against store "${store.id}" failed; ` + + 'falling back to the next configured store', + error, + ) + } + } + } + throw lastError instanceof Error + ? lastError + : new Error(String(lastError as unknown)) } export default class DicomWebManager implements dwc.api.DICOMwebClient { @@ -44,13 +171,22 @@ export default class DicomWebManager implements dwc.api.DICOMwebClient { this.handleError = onError } else { this.handleError = (error, serverSettings) => { - // Only log errors in development environment if (process.env.NODE_ENV === 'development') { console.error(error, serverSettings) } } } + if (settings.length === 0) { + NotificationMiddleware.onError( + NotificationMiddlewareContext.SLIM, + new CustomError( + errorTypes.COMMUNICATION, + 'At least one server needs to be configured.', + ), + ) + } + settings.forEach((serverSettings) => { if (serverSettings === undefined) { NotificationMiddleware.onError( @@ -129,27 +265,24 @@ export default class DicomWebManager implements dwc.api.DICOMwebClient { write: serverSettings.write ?? false, read: serverSettings.read ?? true, client: new dwc.api.DICOMwebClient(clientSettings), + settings: serverSettings, }) }) - - if (this.stores.length > 1) { - NotificationMiddleware.onError( - NotificationMiddlewareContext.SLIM, - new CustomError( - errorTypes.COMMUNICATION, - 'Only one store is supported for now.', - ), - ) - } } get baseURL(): string { return this.stores[0].client.baseURL } + /** + * Update auth (or other) headers on every wrapped store so token refreshes + * propagate to the primary AND secondary endpoints. + */ updateHeaders = (fields: { [name: string]: string }): void => { for (const f in fields) { - this.stores[0].client.headers[f] = fields[f] + for (const store of this.stores) { + store.client.headers[f] = fields[f] + } } } @@ -157,39 +290,71 @@ export default class DicomWebManager implements dwc.api.DICOMwebClient { return this.stores[0].client.headers } + /** + * Store new instances in the first writable configured store. Picking the + * first writable (rather than always store[0]) keeps backwards compatibility + * with single-store deployments while letting STOW route to the secondary + * (`gcp=` URL) annotation store when present. + */ storeInstances = async ( options: dwc.api.StoreInstancesOptions, ): Promise => { - if (this.stores[0].write) { - return await this.stores[0].client.storeInstances(options) - } else { + const writable = this.stores.find((s) => s.write) + if (writable === undefined) { return await Promise.reject(new Error('Store is not writable.')) } + return await writable.client.storeInstances(options) } searchForStudies = async ( options: dwc.api.SearchForStudiesOptions, ): Promise => { - return await this.stores[0].client.searchForStudies(options) + const merged = await searchAcrossStores( + this.stores, + [STUDY_INSTANCE_UID_TAG], + async (store) => + (await store.client.searchForStudies( + options, + )) as unknown as DicomJsonObject[], + ) + return merged as unknown as dwc.api.Study[] } searchForSeries = async ( options: dwc.api.SearchForSeriesOptions, ): Promise => { - return await this.stores[0].client.searchForSeries(options) + const merged = await searchAcrossStores( + this.stores, + [STUDY_INSTANCE_UID_TAG, SERIES_INSTANCE_UID_TAG], + async (store) => + (await store.client.searchForSeries( + options, + )) as unknown as DicomJsonObject[], + ) + return merged as unknown as dwc.api.Series[] } searchForInstances = async ( options: dwc.api.SearchForInstancesOptions, ): Promise => { - return await this.stores[0].client.searchForInstances(options) + const merged = await searchAcrossStores( + this.stores, + [STUDY_INSTANCE_UID_TAG, SERIES_INSTANCE_UID_TAG, SOP_INSTANCE_UID_TAG], + async (store) => + (await store.client.searchForInstances( + options, + )) as unknown as DicomJsonObject[], + ) + return merged as unknown as dwc.api.Instance[] } retrieveStudyMetadata = async ( options: dwc.api.RetrieveStudyMetadataOptions, ): Promise => { - const studySummaryMetadata = - await this.stores[0].client.retrieveStudyMetadata(options) + const studySummaryMetadata = await retrieveWithFallback( + this.stores, + async (store) => await store.client.retrieveStudyMetadata(options), + ) const naturalized = naturalizeDataset(studySummaryMetadata) DicomMetadataStore.addStudy(naturalized as Record) return studySummaryMetadata @@ -198,8 +363,10 @@ export default class DicomWebManager implements dwc.api.DICOMwebClient { retrieveSeriesMetadata = async ( options: dwc.api.RetrieveSeriesMetadataOptions, ): Promise => { - const seriesSummaryMetadata = - await this.stores[0].client.retrieveSeriesMetadata(options) + const seriesSummaryMetadata = await retrieveWithFallback( + this.stores, + async (store) => await store.client.retrieveSeriesMetadata(options), + ) const naturalized = seriesSummaryMetadata.map(naturalizeDataset) DicomMetadataStore.addSeriesMetadata( naturalized as Array>, @@ -211,13 +378,19 @@ export default class DicomWebManager implements dwc.api.DICOMwebClient { retrieveInstanceMetadata = async ( options: dwc.api.RetrieveInstanceMetadataOptions, ): Promise => { - return await this.stores[0].client.retrieveInstanceMetadata(options) + return await retrieveWithFallback( + this.stores, + async (store) => await store.client.retrieveInstanceMetadata(options), + ) } retrieveInstance = async ( options: dwc.api.RetrieveInstanceOptions, ): Promise => { - const instance = await this.stores[0].client.retrieveInstance(options) + const instance = await retrieveWithFallback( + this.stores, + async (store) => await store.client.retrieveInstance(options), + ) const data = dcmjs.data.DicomMessage.readFile(instance) const { dataset } = dmv.metadata.formatMetadata(data.dict) DicomMetadataStore.addInstances([dataset as Instance]) @@ -227,24 +400,37 @@ export default class DicomWebManager implements dwc.api.DICOMwebClient { retrieveInstanceFrames = async ( options: dwc.api.RetrieveInstanceFramesOptions, ): Promise => { - return await this.stores[0].client.retrieveInstanceFrames(options) + return await retrieveWithFallback( + this.stores, + async (store) => await store.client.retrieveInstanceFrames(options), + ) } retrieveInstanceRendered = async ( options: dwc.api.RetrieveInstanceRenderedOptions, ): Promise => { - return await this.stores[0].client.retrieveInstanceRendered(options) + return await retrieveWithFallback( + this.stores, + async (store) => await store.client.retrieveInstanceRendered(options), + ) } retrieveInstanceFramesRendered = async ( options: dwc.api.RetrieveInstanceFramesRenderedOptions, ): Promise => { - return await this.stores[0].client.retrieveInstanceFramesRendered(options) + return await retrieveWithFallback( + this.stores, + async (store) => + await store.client.retrieveInstanceFramesRendered(options), + ) } retrieveBulkData = async ( options: dwc.api.RetrieveBulkDataOptions, ): Promise => { - return await this.stores[0].client.retrieveBulkData(options) + return await retrieveWithFallback( + this.stores, + async (store) => await store.client.retrieveBulkData(options), + ) } } diff --git a/src/__tests__/DicomWebManager.test.ts b/src/__tests__/DicomWebManager.test.ts new file mode 100644 index 00000000..931681f8 --- /dev/null +++ b/src/__tests__/DicomWebManager.test.ts @@ -0,0 +1,336 @@ +// skipcq: JS-C1003 +import * as dwc from 'dicomweb-client' + +import DicomWebManager from '../DicomWebManager' + +interface StubClient { + baseURL: string + headers: Record + searchForStudies: jest.Mock + searchForSeries: jest.Mock + searchForInstances: jest.Mock + retrieveStudyMetadata: jest.Mock + retrieveSeriesMetadata: jest.Mock + retrieveInstance: jest.Mock + retrieveInstanceMetadata: jest.Mock + retrieveInstanceFrames: jest.Mock + retrieveInstanceRendered: jest.Mock + retrieveInstanceFramesRendered: jest.Mock + retrieveBulkData: jest.Mock + storeInstances: jest.Mock +} + +interface ManagerInternals { + stores: Array<{ + id: string + read: boolean + write: boolean + client: StubClient + }> +} + +const makeStubClient = (id: string): StubClient => ({ + baseURL: `https://example.test/${id}/dicomWeb`, + headers: {}, + searchForStudies: jest.fn(), + searchForSeries: jest.fn(), + searchForInstances: jest.fn(), + retrieveStudyMetadata: jest.fn(), + retrieveSeriesMetadata: jest.fn(), + retrieveInstance: jest.fn(), + retrieveInstanceMetadata: jest.fn(), + retrieveInstanceFrames: jest.fn(), + retrieveInstanceRendered: jest.fn(), + retrieveInstanceFramesRendered: jest.fn(), + retrieveBulkData: jest.fn(), + storeInstances: jest.fn(), +}) + +/** + * Replace the underlying dwc client of every store with a stub so we can test + * the multi-store search/retrieve semantics without making real HTTP requests. + */ +const stubManagerClients = ( + manager: DicomWebManager, + stubs: StubClient[], +): void => { + const internals = manager as unknown as ManagerInternals + expect(internals.stores.length).toBe(stubs.length) + internals.stores.forEach((store, i) => { + store.client = stubs[i] + }) +} + +const baseUri = 'https://example.test' + +describe('DicomWebManager - multi-store search', () => { + it('merges searchForSeries results across stores and de-duplicates by SeriesInstanceUID', async () => { + const manager = new DicomWebManager({ + baseUri, + settings: [ + { id: 'primary', url: 'https://primary.test/dicomWeb', write: false }, + { id: 'secondary', url: 'https://secondary.test/dicomWeb', write: false }, + ], + }) + + const primaryStub = makeStubClient('primary') + const secondaryStub = makeStubClient('secondary') + stubManagerClients(manager, [primaryStub, secondaryStub]) + + primaryStub.searchForSeries.mockResolvedValue([ + { + '0020000D': { vr: 'UI', Value: ['1.2.3'] }, + '0020000E': { vr: 'UI', Value: ['1.2.3.A'] }, + '00080060': { vr: 'CS', Value: ['ANN'] }, + }, + // Same SeriesInstanceUID appears in both stores; should be deduped. + { + '0020000D': { vr: 'UI', Value: ['1.2.3'] }, + '0020000E': { vr: 'UI', Value: ['1.2.3.SHARED'] }, + '00080060': { vr: 'CS', Value: ['SR'] }, + }, + ]) + secondaryStub.searchForSeries.mockResolvedValue([ + { + '0020000D': { vr: 'UI', Value: ['1.2.3'] }, + '0020000E': { vr: 'UI', Value: ['1.2.3.B'] }, + '00080060': { vr: 'CS', Value: ['PM'] }, + }, + { + '0020000D': { vr: 'UI', Value: ['1.2.3'] }, + '0020000E': { vr: 'UI', Value: ['1.2.3.SHARED'] }, + '00080060': { vr: 'CS', Value: ['SR'] }, + }, + ]) + + const merged = await manager.searchForSeries({ + studyInstanceUID: '1.2.3', + } as dwc.api.SearchForSeriesOptions) + + expect(primaryStub.searchForSeries).toHaveBeenCalledTimes(1) + expect(secondaryStub.searchForSeries).toHaveBeenCalledTimes(1) + expect(merged.length).toBe(3) + const seriesUIDs = ( + merged as unknown as Array> + ) + .map((m) => m['0020000E']?.Value?.[0]) + .sort() + expect(seriesUIDs).toEqual(['1.2.3.A', '1.2.3.B', '1.2.3.SHARED']) + }) + + it('still returns results from the other store when one store fails', async () => { + const manager = new DicomWebManager({ + baseUri, + settings: [ + { id: 'primary', url: 'https://primary.test/dicomWeb', write: false }, + { id: 'secondary', url: 'https://secondary.test/dicomWeb', write: false }, + ], + }) + + const primaryStub = makeStubClient('primary') + const secondaryStub = makeStubClient('secondary') + stubManagerClients(manager, [primaryStub, secondaryStub]) + + primaryStub.searchForSeries.mockRejectedValue(new Error('boom')) + secondaryStub.searchForSeries.mockResolvedValue([ + { + '0020000D': { vr: 'UI', Value: ['1.2.3'] }, + '0020000E': { vr: 'UI', Value: ['1.2.3.B'] }, + }, + ]) + + const merged = await manager.searchForSeries({ + studyInstanceUID: '1.2.3', + } as dwc.api.SearchForSeriesOptions) + + expect(merged.length).toBe(1) + expect( + (merged[0] as unknown as Record)[ + '0020000E' + ]?.Value?.[0], + ).toBe('1.2.3.B') + }) + + it('skips stores marked as not readable during searches', async () => { + const manager = new DicomWebManager({ + baseUri, + settings: [ + { + id: 'primary', + url: 'https://primary.test/dicomWeb', + write: false, + read: false, + }, + { id: 'secondary', url: 'https://secondary.test/dicomWeb', write: false }, + ], + }) + + const primaryStub = makeStubClient('primary') + const secondaryStub = makeStubClient('secondary') + stubManagerClients(manager, [primaryStub, secondaryStub]) + + secondaryStub.searchForInstances.mockResolvedValue([ + { + '0020000D': { vr: 'UI', Value: ['1.2.3'] }, + '0020000E': { vr: 'UI', Value: ['1.2.3.B'] }, + '00080018': { vr: 'UI', Value: ['1.2.3.B.1'] }, + }, + ]) + + const merged = await manager.searchForInstances({ + studyInstanceUID: '1.2.3', + } as dwc.api.SearchForInstancesOptions) + + expect(primaryStub.searchForInstances).not.toHaveBeenCalled() + expect(secondaryStub.searchForInstances).toHaveBeenCalledTimes(1) + expect(merged.length).toBe(1) + }) +}) + +describe('DicomWebManager - multi-store retrieve fallback', () => { + it('returns the primary store result when it succeeds without falling back', async () => { + const manager = new DicomWebManager({ + baseUri, + settings: [ + { id: 'primary', url: 'https://primary.test/dicomWeb', write: false }, + { id: 'secondary', url: 'https://secondary.test/dicomWeb', write: false }, + ], + }) + + const primaryStub = makeStubClient('primary') + const secondaryStub = makeStubClient('secondary') + stubManagerClients(manager, [primaryStub, secondaryStub]) + + primaryStub.retrieveInstanceFrames.mockResolvedValue(['frame-from-primary']) + + const frames = await manager.retrieveInstanceFrames({ + studyInstanceUID: '1.2.3', + seriesInstanceUID: '1.2.3.A', + sopInstanceUID: '1.2.3.A.1', + frameNumbers: [1], + } as dwc.api.RetrieveInstanceFramesOptions) + + expect(primaryStub.retrieveInstanceFrames).toHaveBeenCalledTimes(1) + expect(secondaryStub.retrieveInstanceFrames).not.toHaveBeenCalled() + expect(frames).toEqual(['frame-from-primary']) + }) + + it('falls back to the secondary store when the primary store rejects', async () => { + const manager = new DicomWebManager({ + baseUri, + settings: [ + { id: 'primary', url: 'https://primary.test/dicomWeb', write: false }, + { id: 'secondary', url: 'https://secondary.test/dicomWeb', write: false }, + ], + }) + + const primaryStub = makeStubClient('primary') + const secondaryStub = makeStubClient('secondary') + stubManagerClients(manager, [primaryStub, secondaryStub]) + + primaryStub.retrieveInstanceFrames.mockRejectedValue( + Object.assign(new Error('not found'), { status: 404 }), + ) + secondaryStub.retrieveInstanceFrames.mockResolvedValue([ + 'frame-from-secondary', + ]) + + const frames = await manager.retrieveInstanceFrames({ + studyInstanceUID: '1.2.3', + seriesInstanceUID: '1.2.3.B', + sopInstanceUID: '1.2.3.B.1', + frameNumbers: [1], + } as dwc.api.RetrieveInstanceFramesOptions) + + expect(primaryStub.retrieveInstanceFrames).toHaveBeenCalledTimes(1) + expect(secondaryStub.retrieveInstanceFrames).toHaveBeenCalledTimes(1) + expect(frames).toEqual(['frame-from-secondary']) + }) + + it('throws the last error when every store fails', async () => { + const manager = new DicomWebManager({ + baseUri, + settings: [ + { id: 'primary', url: 'https://primary.test/dicomWeb', write: false }, + { id: 'secondary', url: 'https://secondary.test/dicomWeb', write: false }, + ], + }) + + const primaryStub = makeStubClient('primary') + const secondaryStub = makeStubClient('secondary') + stubManagerClients(manager, [primaryStub, secondaryStub]) + + primaryStub.retrieveBulkData.mockRejectedValue(new Error('first')) + secondaryStub.retrieveBulkData.mockRejectedValue(new Error('second')) + + await expect( + manager.retrieveBulkData({ + BulkDataURI: 'https://example.test/bulkdata/1', + } as unknown as dwc.api.RetrieveBulkDataOptions), + ).rejects.toThrow('second') + }) +}) + +describe('DicomWebManager - storeInstances and headers', () => { + it('routes storeInstances to the first writable store even when it is the secondary', async () => { + const manager = new DicomWebManager({ + baseUri, + settings: [ + { id: 'primary', url: 'https://primary.test/dicomWeb', write: false }, + { id: 'secondary', url: 'https://secondary.test/dicomWeb', write: true }, + ], + }) + + const primaryStub = makeStubClient('primary') + const secondaryStub = makeStubClient('secondary') + stubManagerClients(manager, [primaryStub, secondaryStub]) + + secondaryStub.storeInstances.mockResolvedValue(undefined) + + await manager.storeInstances({ + datasets: [], + } as dwc.api.StoreInstancesOptions) + + expect(primaryStub.storeInstances).not.toHaveBeenCalled() + expect(secondaryStub.storeInstances).toHaveBeenCalledTimes(1) + }) + + it('rejects storeInstances when no configured store is writable', async () => { + const manager = new DicomWebManager({ + baseUri, + settings: [ + { id: 'primary', url: 'https://primary.test/dicomWeb', write: false }, + { id: 'secondary', url: 'https://secondary.test/dicomWeb', write: false }, + ], + }) + + const primaryStub = makeStubClient('primary') + const secondaryStub = makeStubClient('secondary') + stubManagerClients(manager, [primaryStub, secondaryStub]) + + await expect( + manager.storeInstances({ + datasets: [], + } as dwc.api.StoreInstancesOptions), + ).rejects.toThrow('Store is not writable.') + }) + + it('propagates updateHeaders to every wrapped store', () => { + const manager = new DicomWebManager({ + baseUri, + settings: [ + { id: 'primary', url: 'https://primary.test/dicomWeb', write: false }, + { id: 'secondary', url: 'https://secondary.test/dicomWeb', write: false }, + ], + }) + + const primaryStub = makeStubClient('primary') + const secondaryStub = makeStubClient('secondary') + stubManagerClients(manager, [primaryStub, secondaryStub]) + + manager.updateHeaders({ Authorization: 'Bearer abc' }) + + expect(primaryStub.headers.Authorization).toBe('Bearer abc') + expect(secondaryStub.headers.Authorization).toBe('Bearer abc') + }) +})