# HG changeset patch # User Alain Mazy # Date 1674061131 -3600 # Node ID 15109c3f0f7d2be0a5623d92f7e442057d194968 # Parent e71b22a43c0bfde04c23d81020c7ae627f10d5dc added sanity checks in DicomModificationJob + automatically reconstruct resources at the end of a DicomModificationJob diff -r e71b22a43c0b -r 15109c3f0f7d NEWS --- a/NEWS Tue Jan 17 17:54:38 2023 +0100 +++ b/NEWS Wed Jan 18 17:58:51 2023 +0100 @@ -21,9 +21,11 @@ - allow modification of PatientID, StudyInstanceUID at series level - allow modification of PatientID, StudyInstanceUID, SeriesInstanceUID at instance level - allow modification of a patient without changing her PatientID - Users should be careful to preserve the DICOM model when modifying high level tags. E.g. - if you modify the PatientID at study level, also make sure to modify all other Patient related - tags (PatientName, PatientBirthDate, ...) + Added sanity checks for modifications to make sure the user preserves the DICOM model when modifying high level tags. + E.g. if you modify the PatientID at study level, also make sure to modify all other Patient related + tags (PatientName, PatientBirthDate, ...) +* Automatically reconstruct the modified resources at the end of the DICOM modifications job to ensure + improved consistency of the DICOM model. * Allow the HTTP server to return responses > 2GB (fixes asynchronous download of zip studies > 2GB) * /modalities/.../store now accepts "CalledAet", "Host", "Port" to override the modality configuration from the configuration file for a specific operation. diff -r e71b22a43c0b -r 15109c3f0f7d OrthancFramework/Sources/DicomParsing/DicomModification.cpp --- a/OrthancFramework/Sources/DicomParsing/DicomModification.cpp Tue Jan 17 17:54:38 2023 +0100 +++ b/OrthancFramework/Sources/DicomParsing/DicomModification.cpp Wed Jan 18 17:58:51 2023 +0100 @@ -544,6 +544,7 @@ void DicomModification::Keep(const DicomTag& tag) { + keep_.insert(tag); removals_.erase(tag); clearings_.erase(tag); uids_.erase(tag); @@ -640,6 +641,11 @@ return replacements_.find(tag) != replacements_.end(); } + bool DicomModification::IsKept(const DicomTag& tag) const + { + return keep_.find(tag) != keep_.end(); + } + const Json::Value& DicomModification::GetReplacement(const DicomTag& tag) const { Replacements::const_iterator it = replacements_.find(tag); @@ -849,6 +855,7 @@ isAnonymization_ = true; + keep_.clear(); removals_.clear(); clearings_.clear(); removedRanges_.clear(); @@ -926,7 +933,7 @@ IsRemoved(DICOM_TAG_SERIES_INSTANCE_UID) || IsRemoved(DICOM_TAG_SOP_INSTANCE_UID)) { - throw OrthancException(ErrorCode_BadRequest); + throw OrthancException(ErrorCode_BadRequest, "It is forbidden to remove one of the main Dicom identifiers"); } if (!allowManualIdentifiers_) @@ -1843,4 +1850,13 @@ (tag == DICOM_TAG_SOP_INSTANCE_UID && !keepSopInstanceUid_)); } + + void DicomModification::GetReplacedTags(std::set& target) const + { + target.clear(); + for (Replacements::const_iterator it = replacements_.begin(); it != replacements_.end(); it++) + { + target.insert(it->first); + } + } } diff -r e71b22a43c0b -r 15109c3f0f7d OrthancFramework/Sources/DicomParsing/DicomModification.h --- a/OrthancFramework/Sources/DicomParsing/DicomModification.h Tue Jan 17 17:54:38 2023 +0100 +++ b/OrthancFramework/Sources/DicomParsing/DicomModification.h Wed Jan 18 17:58:51 2023 +0100 @@ -130,6 +130,7 @@ mutable boost::recursive_mutex uidMapMutex_; SetOfTags removals_; SetOfTags clearings_; + SetOfTags keep_; Replacements replacements_; bool removePrivateTags_; ResourceType level_; @@ -210,10 +211,14 @@ bool IsReplaced(const DicomTag& tag) const; + void GetReplacedTags(std::set& target) const; + const Json::Value& GetReplacement(const DicomTag& tag) const; std::string GetReplacementAsString(const DicomTag& tag) const; + bool IsKept(const DicomTag& tag) const; + void SetRemovePrivateTags(bool removed); bool ArePrivateTagsRemoved() const; diff -r e71b22a43c0b -r 15109c3f0f7d OrthancServer/Sources/OrthancRestApi/OrthancRestAnonymizeModify.cpp --- a/OrthancServer/Sources/OrthancRestApi/OrthancRestAnonymizeModify.cpp Tue Jan 17 17:54:38 2023 +0100 +++ b/OrthancServer/Sources/OrthancRestApi/OrthancRestAnonymizeModify.cpp Wed Jan 18 17:58:51 2023 +0100 @@ -387,7 +387,9 @@ job->AddParentResource(*it); } - + + job->PerformSanityChecks(); + OrthancRestApi::GetApi(call).SubmitThreadedInstancesJob (call, job.release(), true /* synchronous by default */, body); } diff -r e71b22a43c0b -r 15109c3f0f7d OrthancServer/Sources/ServerJobs/ResourceModificationJob.cpp --- a/OrthancServer/Sources/ServerJobs/ResourceModificationJob.cpp Tue Jan 17 17:54:38 2023 +0100 +++ b/OrthancServer/Sources/ServerJobs/ResourceModificationJob.cpp Wed Jan 18 17:58:51 2023 +0100 @@ -168,7 +168,8 @@ boost::recursive_mutex::scoped_lock lock(mutex_); // TODO: cleanup the instances that have been generated during the previous run - modifiedInstances_.clear(); + modifiedSeries_.clear(); + instancesToReconstruct_.clear(); ThreadedSetOfInstancesJob::Reset(); } @@ -178,12 +179,15 @@ boost::recursive_mutex::scoped_lock lock(mutex_); // reconstruct the parents MainDicomTags in case one of them has changed - if (modifiedInstances_.size() > 0) + if (instancesToReconstruct_.size() > 0) { - ServerContext::DicomCacheLocker locker(GetContext(), *(modifiedInstances_.begin())); - ParsedDicomFile& modifiedDicom = locker.GetDicom(); + for (std::set::const_iterator it = instancesToReconstruct_.begin(); it != instancesToReconstruct_.end(); ++it) + { + ServerContext::DicomCacheLocker locker(GetContext(), *it); + ParsedDicomFile& modifiedDicom = locker.GetDicom(); - GetContext().GetIndex().ReconstructInstance(modifiedDicom); + GetContext().GetIndex().ReconstructInstance(modifiedDicom); + } } } @@ -320,7 +324,13 @@ boost::recursive_mutex::scoped_lock lock(outputMutex_); output_->Update(modifiedHasher); - modifiedInstances_.insert(modifiedInstance); + if (modifiedSeries_.find(modifiedHasher.HashSeries()) == modifiedSeries_.end()) + { + modifiedSeries_.insert(modifiedHasher.HashSeries()); + // add an instance to reconstruct for each series + instancesToReconstruct_.insert(modifiedHasher.HashInstance()); + } + } return true; @@ -613,7 +623,7 @@ { value[TRANSCODE] = GetTransferSyntaxUid(transferSyntax_); } - + origin_.Serialize(value[ORIGIN]); Json::Value tmp; @@ -630,4 +640,115 @@ return true; } } + + void ResourceModificationJob::PerformSanityChecks() + { + boost::recursive_mutex::scoped_lock lock(mutex_); // because we access the parentResources_ + + std::set emptyRequestedTags; + + if (modification_.get() == NULL) + { + throw OrthancException(ErrorCode_BadSequenceOfCalls); + } + + bool replacePatientMainDicomTags = false; + bool replaceStudyMainDicomTags = false; + bool replaceSeriesMainDicomTags = false; + bool replaceInstanceMainDicomTags = false; + + ResourceType modificationLevel = modification_->GetLevel(); + std::set replacedTags; + modification_->GetReplacedTags(replacedTags); + + for (std::set::const_iterator it = replacedTags.begin(); it != replacedTags.end(); it++) + { + replacePatientMainDicomTags |= DicomMap::IsMainDicomTag(*it, ResourceType_Patient); + replaceStudyMainDicomTags |= DicomMap::IsMainDicomTag(*it, ResourceType_Study); + replaceSeriesMainDicomTags |= DicomMap::IsMainDicomTag(*it, ResourceType_Series); + replaceInstanceMainDicomTags |= DicomMap::IsMainDicomTag(*it, ResourceType_Instance); + } + + if (modification_->IsKept(DICOM_TAG_SOP_INSTANCE_UID) && !IsKeepSource()) + { + // note: we could refine this criteria -> this is valid only if all DicomUIDs are kept identical (but this can happen through Keep or Replace options) + throw OrthancException(ErrorCode_BadRequest, + "When keeping SOPInstanceUID tag, you must set KeepSource to true to avoid deleting the modified files at the end of the process"); + } + + if (modificationLevel == ResourceType_Study && replacePatientMainDicomTags) + { + for (std::set::const_iterator studyId = parentResources_.begin(); studyId != parentResources_.end(); ++studyId) + { + // When modifying a study, you may not modify patient tags as you wish. + // - If this is the patient's only study, you may modify all patient tags. This could be performed in 2 steps (modify the patient and then, the study) but, + // for many use cases, it's helpful to be able to do it one step (e.g, to modify a name in a study that has just been acquired) + // - If the patient already has other studies, you may only 'attach' the study to an existing patient by modifying + // all patient tags from the study to match those of the target patient. + // - Otherwise, you can't modify the patient tags + + std::string targetPatientId; + if (modification_->IsReplaced(DICOM_TAG_PATIENT_ID)) + { + targetPatientId = modification_->GetReplacementAsString(DICOM_TAG_PATIENT_ID); + } + else + { + ExpandedResource originalStudy; + if (GetContext().GetIndex().ExpandResource(originalStudy, *studyId, ResourceType_Study, emptyRequestedTags, ExpandResourceDbFlags_IncludeMainDicomTags)) + { + targetPatientId = originalStudy.tags_.GetStringValue(DICOM_TAG_PATIENT_ID, "", false); + } + else + { + throw OrthancException(ErrorCode_UnknownResource, "Study not found"); + } + } + + // try to find the targetPatient + std::vector lookupPatientResult; + GetContext().GetIndex().LookupIdentifierExact(lookupPatientResult, ResourceType_Patient, DICOM_TAG_PATIENT_ID, targetPatientId); + + // if the patient exists, check how many child studies it has. + if (lookupPatientResult.size() >= 1) + { + ExpandedResource targetPatient; + + if (GetContext().GetIndex().ExpandResource(targetPatient, lookupPatientResult[0], ResourceType_Patient, emptyRequestedTags, static_cast(ExpandResourceDbFlags_IncludeMainDicomTags | ExpandResourceDbFlags_IncludeChildren))) + { + const std::list childrenIds = targetPatient.childrenIds_; + bool targetPatientHasOtherStudies = childrenIds.size() > 1; + if (childrenIds.size() == 1) + { + targetPatientHasOtherStudies = std::find(childrenIds.begin(), childrenIds.end(), *studyId) == childrenIds.end(); // if the patient has one study that is not the one being modified + } + + if (targetPatientHasOtherStudies) + { + // this is allowed if all patient replacedTags do match the target patient tags + DicomMap targetPatientTags; + targetPatient.tags_.ExtractPatientInformation(targetPatientTags); + + for (std::set::const_iterator mainPatientTag = DicomMap::GetMainDicomTags(ResourceType_Patient).begin(); + mainPatientTag != DicomMap::GetMainDicomTags(ResourceType_Patient).end(); ++mainPatientTag) + { + if (targetPatientTags.HasTag(*mainPatientTag) + && (!modification_->IsReplaced(*mainPatientTag) + || modification_->GetReplacementAsString(*mainPatientTag) != targetPatientTags.GetStringValue(*mainPatientTag, "", false))) + { + throw OrthancException(ErrorCode_BadRequest, std::string("Trying to change patient tags in a study. The Patient already exists and has other studies. All the 'Replace' tags should match the existing patient main dicom tags. Try using /patients/../modify instead to modify the patient. Failing tag: ") + mainPatientTag->Format()); + } + else if (!targetPatientTags.HasTag(*mainPatientTag) && modification_->IsReplaced(*mainPatientTag) ) + { + throw OrthancException(ErrorCode_BadRequest, std::string("Trying to change patient tags in a study. The Patient already exists and has other studies. You are trying to replace a tag that is not defined yet in this patient. Try using /patients/../modify instead to modify the patient. Failing tag: ") + mainPatientTag->Format()); + } + } + } + // TODO: need reconstruct_ + } + } + } + + } + } } diff -r e71b22a43c0b -r 15109c3f0f7d OrthancServer/Sources/ServerJobs/ResourceModificationJob.h --- a/OrthancServer/Sources/ServerJobs/ResourceModificationJob.h Tue Jan 17 17:54:38 2023 +0100 +++ b/OrthancServer/Sources/ServerJobs/ResourceModificationJob.h Wed Jan 18 17:58:51 2023 +0100 @@ -60,8 +60,8 @@ DicomInstanceOrigin origin_; bool transcode_; DicomTransferSyntax transferSyntax_; - std::set modifiedInstances_; // the list of new instance ids of the newly generated instances - + std::set modifiedSeries_; // the list of new series ids of the newly generated series + std::set instancesToReconstruct_; // for each new series generated, an instance id that we can use to reconstruct the hierarchy DB model protected: virtual bool HandleInstance(const std::string& instance) ORTHANC_OVERRIDE; // from ThreadedSetOfInstancesJob @@ -127,5 +127,7 @@ virtual bool Serialize(Json::Value& value) ORTHANC_OVERRIDE; virtual void Reset() ORTHANC_OVERRIDE; + + void PerformSanityChecks(); }; } diff -r e71b22a43c0b -r 15109c3f0f7d OrthancServer/Sources/ServerJobs/ThreadedSetOfInstancesJob.cpp --- a/OrthancServer/Sources/ServerJobs/ThreadedSetOfInstancesJob.cpp Tue Jan 17 17:54:38 2023 +0100 +++ b/OrthancServer/Sources/ServerJobs/ThreadedSetOfInstancesJob.cpp Wed Jan 18 17:58:51 2023 +0100 @@ -459,6 +459,12 @@ keepSource_ = keep; } + bool ThreadedSetOfInstancesJob::IsKeepSource() const + { + boost::recursive_mutex::scoped_lock lock(mutex_); + + return keepSource_; + } float ThreadedSetOfInstancesJob::GetProgress() { diff -r e71b22a43c0b -r 15109c3f0f7d OrthancServer/Sources/ServerJobs/ThreadedSetOfInstancesJob.h --- a/OrthancServer/Sources/ServerJobs/ThreadedSetOfInstancesJob.h Tue Jan 17 17:54:38 2023 +0100 +++ b/OrthancServer/Sources/ServerJobs/ThreadedSetOfInstancesJob.h Wed Jan 18 17:58:51 2023 +0100 @@ -52,8 +52,6 @@ std::set failedInstances_; // the list of source instances ids that failed processing std::set processedInstances_; // the list of source instances ids that have been processed (including failed ones) - std::set parentResources_; - SharedMessageQueue instancesToProcessQueue_; std::vector > instancesWorkers_; @@ -68,8 +66,10 @@ ServerContext& context_; bool keepSource_; ErrorCode errorCode_; + protected: - mutable boost::recursive_mutex mutex_; + mutable boost::recursive_mutex mutex_; + std::set parentResources_; public: ThreadedSetOfInstancesJob(ServerContext& context, @@ -117,6 +117,8 @@ void SetKeepSource(bool keep); + bool IsKeepSource() const; + void GetInstances(std::set& target) const; void GetFailedInstances(std::set& target) const;