Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions extensions/default/src/DicomWebDataSource/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import { retrieveStudyMetadata, deleteStudyMetadataPromise } from './retrieveStu
import StaticWadoClient from './utils/StaticWadoClient';
import getDirectURL from '../utils/getDirectURL';
import { fixBulkDataURI } from './utils/fixBulkDataURI';
import { applyServiceUrls } from './utils/applyServiceUrls';
import {HeadersInterface} from '@ohif/core/src/types/RequestHeaders';

const { DicomMetaDictionary, DicomDict } = dcmjs.data;
Expand All @@ -35,6 +36,7 @@ export type DicomWebConfig = {
/** Base URL to use for QIDO requests */
qidoRoot?: string;
wadoRoot?: string; // - Base URL to use for WADO requests
stowRoot?: string; // - Base URL to use for STOW requests (defaults to wadoRoot)
wadoUri?: string; // - Base URL to use for WADO URI requests
qidoSupportsIncludeField?: boolean; // - Whether QIDO supports the "Include" option to request additional fields in response
imageRendering?: string; // - wadors | ? (unsure of where/how this is used)
Expand Down Expand Up @@ -207,6 +209,8 @@ function createDicomWebApi(dicomWebConfig: DicomWebConfig, servicesManager) {
wadoDicomWebClient = dicomWebConfig.staticWado
? new StaticWadoClient(wadoConfig)
: new api.DICOMwebClient(wadoConfig);

applyServiceUrls(qidoDicomWebClient, wadoDicomWebClient, dicomWebConfig);
},
query: {
studies: {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
import { applyServiceUrls } from './applyServiceUrls';

describe('applyServiceUrls', () => {
it('should fix cross-service URLs when roots differ', () => {
const qidoClient = { wadoURL: 'https://server.com/qidors/org1', stowURL: 'https://server.com/qidors/org1' };
const wadoClient = { qidoURL: 'https://server.com/wadors/org1', stowURL: 'https://server.com/wadors/org1' };

applyServiceUrls(qidoClient, wadoClient, {
qidoRoot: 'https://server.com/qidors/org1',
wadoRoot: 'https://server.com/wadors/org1',
});

expect(qidoClient.wadoURL).toBe('https://server.com/wadors/org1');
expect(qidoClient.stowURL).toBe('https://server.com/wadors/org1');

expect(wadoClient.qidoURL).toBe('https://server.com/qidors/org1');
expect(wadoClient.stowURL).toBe('https://server.com/wadors/org1');
});

it('should use explicit stowRoot when provided', () => {
const qidoClient = { wadoURL: '', stowURL: '' };
const wadoClient = { qidoURL: '', stowURL: '' };

applyServiceUrls(qidoClient, wadoClient, {
qidoRoot: 'https://server.com/qidors/org1',
wadoRoot: 'https://server.com/wadors/org1',
stowRoot: 'https://server.com/stowrs/org1',
});

expect(qidoClient.stowURL).toBe('https://server.com/stowrs/org1');
expect(wadoClient.stowURL).toBe('https://server.com/stowrs/org1');
});

it('should be a no-op when qidoRoot and wadoRoot are the same', () => {
const qidoClient = { wadoURL: 'https://server.com/dicomweb', stowURL: 'https://server.com/dicomweb' };
const wadoClient = { qidoURL: 'https://server.com/dicomweb', stowURL: 'https://server.com/dicomweb' };

applyServiceUrls(qidoClient, wadoClient, {
qidoRoot: 'https://server.com/dicomweb',
wadoRoot: 'https://server.com/dicomweb',
});

expect(qidoClient.wadoURL).toBe('https://server.com/dicomweb');
expect(qidoClient.stowURL).toBe('https://server.com/dicomweb');
expect(wadoClient.qidoURL).toBe('https://server.com/dicomweb');
expect(wadoClient.stowURL).toBe('https://server.com/dicomweb');
});

it('should default stowURL to wadoRoot when stowRoot is not provided', () => {
const qidoClient = { wadoURL: '', stowURL: '' };
const wadoClient = { qidoURL: '', stowURL: '' };

applyServiceUrls(qidoClient, wadoClient, {
qidoRoot: 'https://server.com/qidors',
wadoRoot: 'https://server.com/wadors',
});

expect(qidoClient.stowURL).toBe('https://server.com/wadors');
expect(wadoClient.stowURL).toBe('https://server.com/wadors');
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
/**
* Ensures each DICOMweb client has the correct URLs for all services.
*
* The dicomweb-client library sets qidoURL, wadoURL, and stowURL all to the
* base `url` when no prefixes are provided. This function cross-assigns the
* correct service URLs so requests are routed to the right endpoint when
* qidoRoot and wadoRoot differ (e.g. /qidors/ vs /wadors/).
*/
export function applyServiceUrls(
qidoClient: { wadoURL: string; stowURL: string },
wadoClient: { qidoURL: string; stowURL: string },
config: { qidoRoot?: string; wadoRoot?: string; stowRoot?: string }
): void {
const effectiveStowRoot = config.stowRoot ?? config.wadoRoot;

if (config.wadoRoot !== undefined) {
qidoClient.wadoURL = config.wadoRoot;
qidoClient.stowURL = effectiveStowRoot;
}

if (config.qidoRoot !== undefined) {
wadoClient.qidoURL = config.qidoRoot;
wadoClient.stowURL = effectiveStowRoot;
}
Comment on lines +14 to +24
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 When config.qidoRoot is defined but both config.wadoRoot and config.stowRoot are undefined, effectiveStowRoot resolves to undefined. The second if block guard only checks config.qidoRoot !== undefined, so wadoClient.stowURL is overwritten with undefined, corrupting the client state. TypeScript strict mode also flags the assignment since effectiveStowRoot: string | undefined is not assignable to stowURL: string. Moving effectiveStowRoot inside each guard (where the relevant root is guaranteed defined) eliminates both the runtime risk and the type error.

Suggested change
const effectiveStowRoot = config.stowRoot ?? config.wadoRoot;
if (config.wadoRoot !== undefined) {
qidoClient.wadoURL = config.wadoRoot;
qidoClient.stowURL = effectiveStowRoot;
}
if (config.qidoRoot !== undefined) {
wadoClient.qidoURL = config.qidoRoot;
wadoClient.stowURL = effectiveStowRoot;
}
if (config.wadoRoot !== undefined) {
const effectiveStowRoot = config.stowRoot ?? config.wadoRoot;
qidoClient.wadoURL = config.wadoRoot;
qidoClient.stowURL = effectiveStowRoot;
}
if (config.qidoRoot !== undefined) {
const effectiveStowRoot = config.stowRoot ?? config.wadoRoot;
wadoClient.qidoURL = config.qidoRoot;
wadoClient.stowURL = effectiveStowRoot;
}
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/default/src/DicomWebDataSource/utils/applyServiceUrls.ts
Line: 14-24

Comment:
When `config.qidoRoot` is defined but both `config.wadoRoot` and `config.stowRoot` are undefined, `effectiveStowRoot` resolves to `undefined`. The second `if` block guard only checks `config.qidoRoot !== undefined`, so `wadoClient.stowURL` is overwritten with `undefined`, corrupting the client state. TypeScript strict mode also flags the assignment since `effectiveStowRoot: string | undefined` is not assignable to `stowURL: string`. Moving `effectiveStowRoot` inside each guard (where the relevant root is guaranteed defined) eliminates both the runtime risk and the type error.

```suggestion
  if (config.wadoRoot !== undefined) {
    const effectiveStowRoot = config.stowRoot ?? config.wadoRoot;
    qidoClient.wadoURL = config.wadoRoot;
    qidoClient.stowURL = effectiveStowRoot;
  }

  if (config.qidoRoot !== undefined) {
    const effectiveStowRoot = config.stowRoot ?? config.wadoRoot;
    wadoClient.qidoURL = config.qidoRoot;
    wadoClient.stowURL = effectiveStowRoot;
  }
```

How can I resolve this? If you propose a fix, please make it concise.

}