--- 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.

--- 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<DicomTag>& target) const
+  {
+    target.clear();
+    for (Replacements::const_iterator it = replacements_.begin(); it != replacements_.end(); it++)
+    {
+      target.insert(it->first);
+    }
+  }
 }

--- 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<DicomTag>& 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;

--- 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);
   }

--- 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<std::string>::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<DicomTag> 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<DicomTag> replacedTags;
+    modification_->GetReplacedTags(replacedTags);
+
+    for (std::set<DicomTag>::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<std::string>::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<std::string> 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>(ExpandResourceDbFlags_IncludeMainDicomTags | ExpandResourceDbFlags_IncludeChildren)))
+          {
+            const std::list<std::string> 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<DicomTag>::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_
+          }
+        }
+      }
+      
+    }
+  }
 }

--- 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<std::string>               modifiedInstances_;   // the list of new instance ids of the newly generated instances
-
+    std::set<std::string>               modifiedSeries_;          // the list of new series ids of the newly generated series
+    std::set<std::string>               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();
   };
 }

--- 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()
   {

--- 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<std::string>               failedInstances_;     // the list of source instances ids that failed processing
     std::set<std::string>               processedInstances_;  // the list of source instances ids that have been processed (including failed ones)
 
-    std::set<std::string>               parentResources_;
-    
     SharedMessageQueue                  instancesToProcessQueue_;
     std::vector<boost::shared_ptr<boost::thread> >         instancesWorkers_;
 
@@ -68,8 +66,10 @@
     ServerContext&          context_;
     bool                    keepSource_;
     ErrorCode               errorCode_;
+  
   protected:
-    mutable boost::recursive_mutex mutex_;
+    mutable boost::recursive_mutex      mutex_;
+    std::set<std::string>               parentResources_;
 
   public:
     ThreadedSetOfInstancesJob(ServerContext& context,
@@ -117,6 +117,8 @@
 
     void SetKeepSource(bool keep);
 
+    bool IsKeepSource() const;
+
     void GetInstances(std::set<std::string>& target) const;
 
     void GetFailedInstances(std::set<std::string>& target) const;