fix(DicomWebDataSource): set correct cross-service URLs on DICOMweb clients#6009
Open
galenzo17 wants to merge 5 commits into
Open
fix(DicomWebDataSource): set correct cross-service URLs on DICOMweb clients#6009galenzo17 wants to merge 5 commits into
galenzo17 wants to merge 5 commits into
Conversation
❌ Deploy Preview for ohif-dev failed. Why did it fail? →
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+27
to
+42
| * Applies the URL fix from index.ts lines ~211-219. | ||
| * Extracted here to test in isolation. | ||
| */ | ||
| function applyServiceUrls( | ||
| qidoClient: MockDICOMwebClient, | ||
| wadoClient: MockDICOMwebClient, | ||
| config: { qidoRoot: string; wadoRoot: string; stowRoot?: string } | ||
| ) { | ||
| const effectiveStowRoot = config.stowRoot || config.wadoRoot; | ||
|
|
||
| qidoClient.wadoURL = config.wadoRoot; | ||
| qidoClient.stowURL = effectiveStowRoot; | ||
|
|
||
| wadoClient.qidoURL = config.qidoRoot; | ||
| wadoClient.stowURL = effectiveStowRoot; | ||
| } |
Contributor
There was a problem hiding this comment.
applyServiceUrls is a hand-copied extract of the assignment block in index.ts. If the production logic changes (e.g., a guard is added, a URL property is renamed, or a new client is introduced), these tests will continue to pass while the real behavior regresses silently. The tests should import and exercise the actual createDicomWebApi initializer (or at minimum import a shared utility), not duplicate its logic inline.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/default/src/DicomWebDataSource/index.test.ts
Line: 27-42
Comment:
**Test logic decoupled from production code**
`applyServiceUrls` is a hand-copied extract of the assignment block in `index.ts`. If the production logic changes (e.g., a guard is added, a URL property is renamed, or a new client is introduced), these tests will continue to pass while the real behavior regresses silently. The tests should import and exercise the actual `createDicomWebApi` initializer (or at minimum import a shared utility), not duplicate its logic inline.
How can I resolve this? If you propose a fix, please make it concise.
…lients The dicomweb-client library sets qidoURL, wadoURL, and stowURL all to the same base url when no prefixes are provided. This causes requests to be routed to the wrong endpoint when qidoRoot and wadoRoot differ (e.g. /qidors/ vs /wadors/). Extract applyServiceUrls into a shared utility so the production code and tests exercise the same function. After constructing the clients, call applyServiceUrls to assign the correct cross-service URLs. Also adds an optional stowRoot config field for deployments with a separate STOW endpoint. Closes OHIF#5820 Signed-off-by: Agustin Bereciartua <bereciartua.agustin@gmail.com>
galenzo17
force-pushed
the
fix/dicomweb-cross-service-urls
branch
from
e8dc14f to
37ef346
Compare
…h coalescing Use ?? instead of || for stowRoot fallback to respect explicit empty strings. Wrap assignments in undefined checks so client URLs are not overwritten when a root is not provided. Signed-off-by: Agustin Bereciartua <bereciartua.agustin@gmail.com>
… avoid undefined stowURL Move effectiveStowRoot computation inside each conditional block so it cannot leak an undefined value when only one root is configured. In the qidoRoot guard, chain the fallback as stowRoot ?? wadoRoot ?? qidoRoot to handle the case where wadoRoot is undefined. Add test covering the edge case of qidoRoot-only configuration.
…s unconditionally When stowRoot and wadoRoot are set but qidoRoot is absent, the second guard block does not run and wadoClient.stowURL stays at its constructor default. Since storeInstances() uses wadoDicomWebClient, the explicit stowRoot was silently ignored. Add an unconditional block that applies stowRoot to both clients whenever it is defined.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
wadoURL,qidoURL,stowURL) on each DICOMweb client so requests are routed to the right endpoint whenqidoRootandwadoRootdiffer (e.g./qidors/vs/wadors/)stowRootconfig field for deployments with a separate STOW endpointstowRoot, same-root (no-op), andstowRootfallback scenariosCloses #5820
Test plan
qidoRootandwadoRootdifferstowRootis used when providedqidoRoot === wadoRootstowURLdefaults towadoRootwhenstowRootis omittedGreptile Summary
This PR extracts a new
applyServiceUrlsutility that cross-assignswadoURL,qidoURL, andstowURLon each DICOMweb client after construction, fixing routing bugs whenqidoRootandwadoRootdiffer. It also adds an optionalstowRootconfig field for deployments with a dedicated STOW endpoint.applyServiceUrls.ts: three guarded blocks handle all root combinations —wadoRoot-only updatesqidoClient,qidoRoot-only updateswadoClient, and an independentstowRootblock ensures both clients receive the explicit STOW URL even whenqidoRootis absent (the edge case flagged in prior review rounds).index.ts: minimal integration — addsstowRoottoDicomWebConfigand callsapplyServiceUrlsimmediately after both clients are constructed ininitialize().applyServiceUrls.test.ts: directly imports the utility and covers all six scenarios including thestowRoot-without-qidoRootedge case.Confidence Score: 5/5
Safe to merge — the cross-service URL assignment is correct across all root combinations, the
??operator prevents silent fallback on empty-string inputs, and the third block correctly handles thestowRoot-without-qidoRootcase that caused a silent no-op in earlier iterations.All issues raised in prior review rounds (falsy
||check, unconditional undefined assignment,wadoClient.stowURLnot updated whenqidoRootis absent) are resolved. The three-block structure is logically complete and all six test scenarios, including the previously uncovered edge case, pass.No files require special attention.
Important Files Changed
Reviews (6): Last reviewed commit: "fix(DicomWebDataSource): ensure explicit..." | Re-trigger Greptile