Mercurial > hg > orthanc

changeset 5224:feba2b0e91bc

Find changesets by keywords (author, files, the commit message), revision number or hash, or revset expression.
Fix a crash in /tools/reconstruct triggered by the Housekeeper plugin when only changing the StorageCompression.
author Alain Mazy <am@osimis.io>
date Mon, 03 Apr 2023 22:19:42 +0200
parents a47b24f231d0
children 5874e5dd9a38
files NEWS OrthancServer/Sources/Database/StatelessDatabaseOperations.cpp OrthancServer/Sources/ServerToolbox.cpp
diffstat 3 files changed, 80 insertions(+), 66 deletions(-) [+]
line wrap: on
line diff
--- a/NEWS	Mon Apr 03 15:13:58 2023 +0200
+++ b/NEWS	Mon Apr 03 22:19:42 2023 +0200
@@ -20,6 +20,8 @@
 * Fix issue 214: VOILUTSequence is not returned in Wado-RS
 * Fix /tools/reset crashing when ExtraMainDicomTags were defined
 * Fix Housekeeper plugin infinite loop if Orthanc is empty.
+* Fix a crash in /tools/reconstruct triggered by the Housekeeper plugin 
+  when only changing the StorageCompression.
 
 
 Version 1.11.3 (2023-02-03)
--- a/OrthancServer/Sources/Database/StatelessDatabaseOperations.cpp	Mon Apr 03 15:13:58 2023 +0200
+++ b/OrthancServer/Sources/Database/StatelessDatabaseOperations.cpp	Mon Apr 03 22:19:42 2023 +0200
@@ -2956,6 +2956,10 @@
                                       MetadataType metadata,
                                       const std::string& value)
       {
+        if (metadata == 15)
+        {
+          LOG(INFO) << "toto";
+        }
         content.AddMetadata(instance, metadata, value);
         instanceMetadata[metadata] = value;
       }
@@ -3081,13 +3085,21 @@
         {
           IDatabaseWrapper::CreateInstanceResult status;
           int64_t instanceId;
+          
+          bool isNewInstance = transaction.CreateInstance(status, instanceId, hashPatient_,
+                                                          hashStudy_, hashSeries_, hashInstance_);
+
+          if (isReconstruct_ && isNewInstance)
+          {
+            // In case of reconstruct, we just want to modify the attachments and some metadata like the TransferSyntex
+            // The DicomTags and many metadata have already been updated before we get here in ReconstructInstance
+            throw OrthancException(ErrorCode_InternalError, "New instance while reconstructing; this should not happen.");
+          }
 
           // Check whether this instance is already stored
-          if (!transaction.CreateInstance(status, instanceId, hashPatient_,
-                                          hashStudy_, hashSeries_, hashInstance_))
+          if (!isNewInstance && !isReconstruct_)
           {
             // The instance already exists
-        
             if (overwrite_)
             {
               // Overwrite the old instance
@@ -3098,7 +3110,7 @@
               if (!transaction.CreateInstance(status, instanceId, hashPatient_,
                                               hashStudy_, hashSeries_, hashInstance_))
               {
-                throw OrthancException(ErrorCode_InternalError);
+                throw OrthancException(ErrorCode_InternalError, "No new instance while overwriting; this should not happen.");
               }
             }
             else
@@ -3173,14 +3185,19 @@
           for (Attachments::const_iterator it = attachments_.begin();
                it != attachments_.end(); ++it)
           {
+            if (isReconstruct_)
+            {
+              // we are replacing attachments during a reconstruction
+              transaction.DeleteAttachment(instanceId, it->GetContentType());
+            }
+
             transaction.AddAttachment(instanceId, *it, 0 /* this is the first revision */);
           }
 
-      
+          if (!isReconstruct_)
           {
             ResourcesContent content(true /* new resource, metadata can be set */);
 
-
             // Attach the user-specified metadata (in case of reconstruction, metadata_ contains all past metadata, including the system ones we want to keep)
             for (MetadataMap::const_iterator 
                    it = metadata_.begin(); it != metadata_.end(); ++it)
@@ -3209,62 +3226,52 @@
               }
             }
 
-            // Populate the tags of the newly-created resources
-
-            content.AddResource(instanceId, ResourceType_Instance, dicomSummary_);
-            SetInstanceMetadata(content, instanceMetadata_, instanceId, MetadataType_MainDicomTagsSignature, DicomMap::GetMainDicomTagsSignature(ResourceType_Instance));  // New in Orthanc 1.11.0
-            SetMainDicomSequenceMetadata(content, instanceId, dicomSummary_, ResourceType_Instance);   // new in Orthanc 1.11.1
-
-            if (status.isNewSeries_)
-            {
-              content.AddResource(status.seriesId_, ResourceType_Series, dicomSummary_);
-              content.AddMetadata(status.seriesId_, MetadataType_MainDicomTagsSignature, DicomMap::GetMainDicomTagsSignature(ResourceType_Series));  // New in Orthanc 1.11.0
-              SetMainDicomSequenceMetadata(content, status.seriesId_, dicomSummary_, ResourceType_Series);   // new in Orthanc 1.11.1
-            }
-
-            if (status.isNewStudy_)
-            {
-              content.AddResource(status.studyId_, ResourceType_Study, dicomSummary_);
-              content.AddMetadata(status.studyId_, MetadataType_MainDicomTagsSignature, DicomMap::GetMainDicomTagsSignature(ResourceType_Study));  // New in Orthanc 1.11.0
-              SetMainDicomSequenceMetadata(content, status.studyId_, dicomSummary_, ResourceType_Study);   // new in Orthanc 1.11.1
-            }
-
-            if (status.isNewPatient_)
+            if (!isReconstruct_)
             {
-              content.AddResource(status.patientId_, ResourceType_Patient, dicomSummary_);
-              content.AddMetadata(status.patientId_, MetadataType_MainDicomTagsSignature, DicomMap::GetMainDicomTagsSignature(ResourceType_Patient));  // New in Orthanc 1.11.0
-              SetMainDicomSequenceMetadata(content, status.patientId_, dicomSummary_, ResourceType_Patient);   // new in Orthanc 1.11.1
-            }
-
-            // Attach the auto-computed metadata for the patient/study/series levels
-            std::string now = SystemToolbox::GetNowIsoString(true /* use UTC time (not local time) */);
-            content.AddMetadata(status.seriesId_, MetadataType_LastUpdate, now);
-            content.AddMetadata(status.studyId_, MetadataType_LastUpdate, now);
-            content.AddMetadata(status.patientId_, MetadataType_LastUpdate, now);
-
-            if (status.isNewSeries_)
-            {
-              if (hasExpectedInstances_)
+              // Populate the tags of the newly-created resources
+              content.AddResource(instanceId, ResourceType_Instance, dicomSummary_);
+              SetInstanceMetadata(content, instanceMetadata_, instanceId, MetadataType_MainDicomTagsSignature, DicomMap::GetMainDicomTagsSignature(ResourceType_Instance));  // New in Orthanc 1.11.0
+              SetMainDicomSequenceMetadata(content, instanceId, dicomSummary_, ResourceType_Instance);   // new in Orthanc 1.11.1
+
+              if (status.isNewSeries_)
+              {
+                content.AddResource(status.seriesId_, ResourceType_Series, dicomSummary_);
+                content.AddMetadata(status.seriesId_, MetadataType_MainDicomTagsSignature, DicomMap::GetMainDicomTagsSignature(ResourceType_Series));  // New in Orthanc 1.11.0
+                SetMainDicomSequenceMetadata(content, status.seriesId_, dicomSummary_, ResourceType_Series);   // new in Orthanc 1.11.1
+              }
+
+              if (status.isNewStudy_)
+              {
+                content.AddResource(status.studyId_, ResourceType_Study, dicomSummary_);
+                content.AddMetadata(status.studyId_, MetadataType_MainDicomTagsSignature, DicomMap::GetMainDicomTagsSignature(ResourceType_Study));  // New in Orthanc 1.11.0
+                SetMainDicomSequenceMetadata(content, status.studyId_, dicomSummary_, ResourceType_Study);   // new in Orthanc 1.11.1
+              }
+
+              if (status.isNewPatient_)
               {
-                content.AddMetadata(status.seriesId_, MetadataType_Series_ExpectedNumberOfInstances,
-                                    boost::lexical_cast<std::string>(expectedInstances_));
+                content.AddResource(status.patientId_, ResourceType_Patient, dicomSummary_);
+                content.AddMetadata(status.patientId_, MetadataType_MainDicomTagsSignature, DicomMap::GetMainDicomTagsSignature(ResourceType_Patient));  // New in Orthanc 1.11.0
+                SetMainDicomSequenceMetadata(content, status.patientId_, dicomSummary_, ResourceType_Patient);   // new in Orthanc 1.11.1
               }
 
-              // New in Orthanc 1.9.0
-              content.AddMetadata(status.seriesId_, MetadataType_RemoteAet,
-                                  origin_.GetRemoteAetC());
-            }
-
-            if (hasTransferSyntax_)
-            {
-              // New in Orthanc 1.2.0
-              SetInstanceMetadata(content, instanceMetadata_, instanceId,
-                                  MetadataType_Instance_TransferSyntax,
-                                  GetTransferSyntaxUid(transferSyntax_));
-            }
-
-            if (!isReconstruct_) // don't change origin metadata
-            {        
+              // Attach the auto-computed metadata for the patient/study/series levels
+              std::string now = SystemToolbox::GetNowIsoString(true /* use UTC time (not local time) */);
+              content.AddMetadata(status.seriesId_, MetadataType_LastUpdate, now);
+              content.AddMetadata(status.studyId_, MetadataType_LastUpdate, now);
+              content.AddMetadata(status.patientId_, MetadataType_LastUpdate, now);
+
+              if (status.isNewSeries_)
+              {
+                if (hasExpectedInstances_)
+                {
+                  content.AddMetadata(status.seriesId_, MetadataType_Series_ExpectedNumberOfInstances,
+                                      boost::lexical_cast<std::string>(expectedInstances_));
+                }
+
+                // New in Orthanc 1.9.0
+                content.AddMetadata(status.seriesId_, MetadataType_RemoteAet,
+                                    origin_.GetRemoteAetC());
+              }
               // Attach the auto-computed metadata for the instance level,
               // reflecting these additions into the input metadata map
               SetInstanceMetadata(content, instanceMetadata_, instanceId,
@@ -3298,6 +3305,17 @@
               }
             }
 
+            // Following metadatas are also updated if reconstructing the instance.
+            // They might be missing since they have been introduced along Orthanc versions.
+
+            if (hasTransferSyntax_)
+            {
+              // New in Orthanc 1.2.0
+              SetInstanceMetadata(content, instanceMetadata_, instanceId,
+                                  MetadataType_Instance_TransferSyntax,
+                                  GetTransferSyntaxUid(transferSyntax_));
+            }
+
             if (hasPixelDataOffset_)
             {
               // New in Orthanc 1.9.1
--- a/OrthancServer/Sources/ServerToolbox.cpp	Mon Apr 03 15:13:58 2023 +0200
+++ b/OrthancServer/Sources/ServerToolbox.cpp	Mon Apr 03 22:19:42 2023 +0200
@@ -281,14 +281,8 @@
           std::string resultPublicId;  // ignored
           std::unique_ptr<DicomInstanceToStore> dicomInstancetoStore(DicomInstanceToStore::CreateFromParsedDicomFile(locker.GetDicom()));
 
-          context.GetIndex().GetAllMetadata(instanceMetadata, *it, ResourceType_Instance);
-          
-          for (InstanceMetadata::const_iterator itm = instanceMetadata.begin();
-              itm != instanceMetadata.end(); ++itm)
-          {
-            dicomInstancetoStore->AddMetadata(ResourceType_Instance, itm->first, itm->second);
-          }
-
+          // TODO: TranscodeAndStore and specifically ServerIndex::Store have been "poluted" by the isReconstruct parameter
+          // we should very likely refactor it
           context.TranscodeAndStore(resultPublicId, dicomInstancetoStore.get(), StoreInstanceMode_OverwriteDuplicate, true);
         }
       }