feat: add optional dcm4che5 DICOM backend with runtime selection5 backend#299
feat: add optional dcm4che5 DICOM backend with runtime selection5 backend#299jessesaga wants to merge 4 commits intoOpenIntegrationEngine:mainfrom
Conversation
Add a dcm4che5 backend for the DICOM connector alongside the existing
dcm4che2 backend. Backend selection is controlled at startup by the new
dicom.library property in mirth.properties ("dcm4che2" default, "dcm4che5"
to opt in). Existing channel configurations work unchanged on either
backend.
Architecture:
- A version-neutral abstraction layer (OieDicomObject, OieDicomElement,
OieDicomSender, OieDicomReceiver, OieDicomConverter, OieVR,
DicomLibraryFactory) decouples connector logic from library specifics.
- Dcm2* implementations wrap the existing dcm4che2 MirthDcmSnd/MirthDcmRcv.
- Dcm5* implementations build an equivalent topology from dcm4che5's
Device/Connection/ApplicationEntity primitives with the same external
behaviour -- source-map keys, C-STORE/C-ECHO handling, storage
commitment, transfer-syntax defaults.
- MirthLauncher resolves a new variant="dicom.library:xxx" attribute on
extension <library> entries so only the configured backend's JARs land
on the classpath at runtime.
Build:
- server/build.xml splits the DICOM extension into three JARs:
dicom-server.jar (version-neutral, always loaded)
dicom-server-dcm2.jar (loaded when dicom.library=dcm4che2)
dicom-server-dcm5.jar (loaded when dicom.library=dcm4che5)
The dcm2 JAR bundles the stock dcm4che-tool-dcmrcv/dcmsnd classes via
zipfileset with duplicate="preserve" so patched classes take priority.
- Adds dcm4che-core-5.34.3.jar and dcm4che-net-5.34.3.jar to
server/lib/extensions/dimse/ and updates THIRD-PARTY-README.txt with
MPL 1.1 attribution for the new version.
Backward compatibility:
- DICOMUtil.byteArrayToDicomObject and dicomObjectToByteArray keep their
public signatures; the return type becomes OieDicomObject, which mirrors
the dcm4che2 DicomObject surface methods (contains, size, isEmpty,
getBytes, getDate, getFloat/getDouble with defaults, getStrings, getInts,
vrOf, nameOf, vm, getNestedDicomObject).
- OieDicomElement.vr() returns an OieVR adapter (code/padding/toString)
matching dcm4che2 VR's runtime shape so scripts calling elem.vr().code()
or String(elem.vr()) continue to work.
- Verified by running identical JavaScript transformer scripts on the
pre-patch baseline and on the patched branch: existing scripts that
work on dcm4che2 continue to work on both backends unchanged.
Tests:
- Unit and integration coverage for the abstraction layer, both backends,
cross-library parity, loopback C-STORE, C-ECHO, error handling, storage
commitment, and TLS with ephemeral self-signed keystores. Integration
tests use ephemeral ports and localhost only; all complete within a
couple of seconds.
- Ready-to-import manual test channels and an API regression script under
server/tests/dicom-channels/ that exercise the full OieDicomObject and
DICOMUtil surface through a live DICOM pipeline.
Documentation:
- docs/dcm4che5-migration-guide.md covers enabling the backend, behavioural
differences, TLS configuration, custom DICOMConfiguration migration,
JAR architecture, verification, and rollback.
Signed-off-by: Jesse Dowell <jesse.dowell@gmail.com>
MirthLauncher already filters extension libraries by their `variant` attribute (e.g., `dicom.library:dcm4che5`) when building the server classpath, but WebStartServlet served all CLIENT/SHARED libraries to the Administrator regardless of variant. That left the Administrator downloading both dcm4che2 and dcm4che5 library JARs whenever both were declared — harmless (different package namespaces), but wasteful and inconsistent with server-side selection. Mirror MirthLauncher.shouldLoadLibrary in WebStartServlet so the Administrator receives only the libraries matching the server's current configuration. Signed-off-by: Jesse Dowell <jesse.dowell@gmail.com>
Followups from a pre-PR audit of the dcm4che5 backend: - Add Object-VR overloads on OieDicomObject for putString / putInt / putBytes / putFragments, delegating via toString(). Preserves backward compatibility for transformer scripts passing a library-specific VR constant (e.g., dcm4che2's VR.PN) when the method surface is the version-neutral wrapper. - Simplify OieDicomObject.getInt(int, int) default from a two-step get(tag) + getInt(tag) lookup to a single contains(tag) + getInt(tag). - Upgrade trace->warn for Dcm5 no-op setters (setFileBufferSize, setTranscoderBufferSize, setDestination). These paths are only reached when the user explicitly set a non-default value, so the warn has zero noise on default configs and surfaces an otherwise-silent ignored setting. Also documents the long-standing upstream behavior of DICOMReceiver 'dest' being ignored on both backends (MirthDcmRcv's onCStoreRQ override streams directly to the channel and never consults DcmRcv.setDestination). - Document no-op settings and DICOMUtil API migration in docs/dcm4che5-migration-guide.md. Signed-off-by: Jesse Dowell <jesse.dowell@gmail.com>
jcdlbs
force-pushed
the
feature/dcm4che5-backend
branch
from
7b6603c to
c9de11f
Compare
jonbartels
left a comment
jonbartels
left a comment
There was a problem hiding this comment.
Should the copyright headers be updated? If the code comes from Mirth, then they are still Mirths copyright. If they are new files generated by the contributors, then having the contributor copyright would be appropriate.
The headers should also be updated to the SPDX format seen elsewhere in the project (bonus points for adding a linter check for this :D )
What real world tests would be most valuable for testing this code?
|
Good point on the Copyright and SPDX format. I'll update the newly added files with the SPDX format and the Saga IT, LLC copyright. What's the policy for the existing files? Convert to SPDX and add an additional FileCopyrightText line? Not sure about the linter but I can take a look :) In terms of real world tests, I'd say the biggest current gaps are around load testing and interop with other PACS. I've done small local load tests but nothing like the volume a real world system might see. It's possible that sustained throughput could reveal some issues. (especially with different threading and queueing settings) I have not yet tested against any real PACS - just Mirth to Mirth communication. It would be good to do some testing against at least Orthanc and dcm4chee-arc. |
|
I'll be the first to call out my ignorance regarding dicom, but is there a reason this is all in the same plugin? Seems like only one version can run at a time, and there's a bunch of proxies to select between the two at runtime (vs loading the legacy dcm4che2 plugin, or loading the alternate dcm4che5 plugin). That presumably introduces a fair bit of debt for go-forward, and change risk for back-compat. |
|
The main reason is to provide an easy upgrade path from the ancient unmaintained dcm4che2 libraries and this seemed like a good way to do it. I'm open to other approaches but this way makes the upgrade relatively painless, the abstraction layer provides a little defense against future dcm4che api changes, and the mirth.properties and runtime library resolution keeps only one of the implementations on the server classpath and client classpath at a time. |
|
Sure. I think the salient questions would be:
The user effect seems neutral: mirth.properties change vs plugin change. |
|
From some hacking around by stripping out the wrappers and dcm4che2 and making v2 vs v5 a separate plugin:
Vs this PR which is at +4,616/-399 for implementation. I call out again: I don't have the DICOM background to evaluate how helpful the wrappers are. My own experience leans towards avoiding wrappers unless they add significant value (since it makes it "OIE DICOM" instead of standard types) - but I can't estimate the value here. |
|
@mgaffigan - The primary goal with the current approach is to ensure that it is perfectly backward compatible to make sure it's an uneventful upgrade process for users.
No, a lot of care was taken to ensure that users that not only just receive and send DICOM can simply upgrade and have things work with dcm4che5. The migration guide in the PR outlines a couple of minor API changes. Most users will be able to switch to dcm4che5 from 2 and have it work without changes. If they are extensively using the mirth DICOM API then it's possible they might have to make very minor adjustments. To find the relevant section in the guide you can search for this phrase - "Only these specific patterns require a one-line change"
Yes, some, but we'd lose Mirth's excellent backward compatibility story. If we just ripped out dcm4che2 and plugged in dcm4che5 the most minimal changes possible many DICOM listener/destination users would be forced to rework their code.
There are very minimal changes that may be required depending on how extensively the DCIOM API is being excercised. The main difference is around java types that are passed but each type is actually duck-type compatible and the javascript engine will be able to resolve correctly. Any explicit casts would have to be updated if they're being used. |
|
That all still seems compatible with having alternate v2 and v5 plugins (with only one installed at a time). I'd suggest this be done that way to avoid reinventing the wheel and making yet another dicom-specific modification to the main server. Using actual plugins has the benefit of:
I defer to your judgement that the wrappers themselves are required to maintain backwards compatibility with user scripts. I'm happy to talk through that in more detail if you would like. Rough process would be:
|
Per PR review feedback from jonbartels: convert all touched files in this PR to the project's SPDX header style (matching the format already in use in LoginStatus.java, DelimitedReader.java, EDIReader.java, etc.). Three patterns applied based on file provenance: * New files contributed by this PR (45 files, mostly under server/src/com/mirth/connect/connectors/dimse/dicom/ and the corresponding test tree): a single Saga IT, LLC copyright line. Replaces the legacy Mirth Corporation block header that was copy-pasted from existing files when these were created. * Existing Mirth files modified by this PR (15 files, including DICOMReceiver, DICOMDispatcher, DICOMUtil, MirthLauncher, etc.): Mirth Corporation copyright preserved, Saga IT, LLC added as a contributor copyright line. * Vendored dcm4che2 tool files modified by this PR (MirthDcmRcv.java, MirthDcmSnd.java): dcm4che project + Mirth Corporation + Saga IT, LLC. Note: the upstream dcm4che2 source carries an MPL-1.1 OR GPL-2.0-only OR LGPL-2.1-only tri-license; we use MPL-2.0 here to match the rest of the project. Surfacing the actual upstream license is a separate concern. Header format matches existing project examples: // SPDX-License-Identifier: MPL-2.0 // SPDX-FileCopyrightText: <holder> CRLF line endings preserved on files that originally used CRLF; LF files unchanged. Per-file diffs are header-only — no source code changed. Signed-off-by: Jesse Dowell <jesse.dowell@gmail.com>
4 participants
I realize this is a large changeset but I'm not sure how this can be done incrementally. I considered splitting it, but the abstraction layer is dead code without a second backend, and the variant-filtered JAR loading only exists to pick between the two. Open to restructuring if there's something that would make review easier.
Why: at Saga IT we've had to work around limitations in the dcm4che2 implementation and we believe the newer dcm4che5 version will have fewer bugs and potentially allow room for performance improvements - not to mention receiving security updates.
Caveat: only tested locally end-to-end, including mTLS DIMSE channels on both backends. This code has not been tested in real world workloads.
Summary
Adds an optional dcm4che5 backend to the DICOM Listener / Sender connector alongside the existing dcm4che2 backend. Selection is per-server via a new
dicom.libraryproperty inmirth.properties. The default remainsdcm4che2- existing installations see zero behavioral changes on upgrade.dcm4che2 has not had a release since 2015. dcm4che5 is actively maintained, has better support for modern Java and TLS, and receives security fixes. This PR gives users the option to move forward without forcing the migration.
Backward compatibility
The default codepath is entirely unchanged. For installations that don't opt in:
dicom.library, unset, defaults todcm4che2DICOMConfigurationclasses continue to load (with fallback logic for pre-PR implementations)DICOMUtil:byteArrayToDicomObject()now returnsOieDicomObjectinstead ofDicomObject. Duck-typed scripts work unchanged thanks to Object-VR overloads. Scripts doing explicit(DicomObject)casts orinstanceof DicomObjectchecks need.unwrap()- documented in the migration guide.Approach
com.mirth.connect.connectors.dimse.dicom:OieDicomObject,OieDicomElement,OieDicomSender,OieDicomReceiver,OieDicomConverter,OieVR. Decouples Mirth's DICOM code from a specific library version.dcm2/wraps existingMirthDcmSnd/MirthDcmRcv(unchanged in behavior).dcm5/composes fromDevice+Connection+ApplicationEntity+ service handlers in idiomatic dcm4che5 style.DicomLibraryFactoryreadsdicom.libraryat startup and instantiates the matching backend via reflection. Default isdcm4che2.variant="dicom.library:<value>"attribute.MirthLauncher(server) andWebStartServlet(Administrator) both honor it, so only the active backend's JARs are loaded on the server and shipped to the Administrator.Testing
server/test/.../dimse/dicom/integration/.Migration
Migration guide for users opting into dcm4che5:
docs/dcm4che5-migration-guide.md. Covers enable/disable, element-name difference (keyword vs PS3.6), async C-STORE dispatch, TLS cipher configuration, and three UI settings that are no-ops on dcm5 (twobufSizetuning flags;dest, which is in fact a silent no-op on dcm4che2 upstream as well).Dependencies
Adds
dcm4che-core-5.34.3anddcm4che-net-5.34.3(MPL-1.1). License entries added toserver/docs/thirdparty/THIRD-PARTY-README.txt.Not in this PR
Manual testing approaches taken beyond test cases
dicom.libraryset): existing DIMSE channels deploy and process messages identically to pre-upgradedicom.library = dcm4che5, restart: channels re-deploy cleanly, C-STORE and C-ECHO behavior verifieddicom.library = dcm4che2, restart: behavior returns to default