# HG changeset patch
# User Sebastien Jodogne <s.jodogne@gmail.com>
# Date 1544785803 -3600
# Node ID abe49ca61cd5dfceec85fc12177c58ab94c9432b
# Parent  b9f0b0c0b36f6e60986623b2d8afc7a94bc814c2
On C-FIND, avoid accessing the storage area whenever possible

diff -r b9f0b0c0b36f -r abe49ca61cd5 Core/DicomFormat/DicomMap.cpp
--- a/Core/DicomFormat/DicomMap.cpp	Thu Dec 13 17:58:27 2018 +0100
+++ b/Core/DicomFormat/DicomMap.cpp	Fri Dec 14 12:10:03 2018 +0100
@@ -1069,6 +1069,25 @@
   }    
 
 
+  bool DicomMap::HasOnlyMainDicomTags() const
+  {
+    // TODO - Speed up possible by making this std::set a global variable
+
+    std::set<DicomTag> mainDicomTags;
+    GetMainDicomTags(mainDicomTags);
+
+    for (Map::const_iterator it = map_.begin(); it != map_.end(); ++it)
+    {
+      if (mainDicomTags.find(it->first) == mainDicomTags.end())
+      {
+        return false;
+      }
+    }
+
+    return true;
+  }
+    
+
   void DicomMap::Serialize(Json::Value& target) const
   {
     target = Json::objectValue;
diff -r b9f0b0c0b36f -r abe49ca61cd5 Core/DicomFormat/DicomMap.h
--- a/Core/DicomFormat/DicomMap.h	Thu Dec 13 17:58:27 2018 +0100
+++ b/Core/DicomFormat/DicomMap.h	Fri Dec 14 12:10:03 2018 +0100
@@ -224,7 +224,9 @@
 
     void Merge(const DicomMap& other);
 
-    void ExtractMainDicomTags(const DicomMap& other); 
+    void ExtractMainDicomTags(const DicomMap& other);
+
+    bool HasOnlyMainDicomTags() const;
     
     void Serialize(Json::Value& target) const;
 
diff -r b9f0b0c0b36f -r abe49ca61cd5 NEWS
--- a/NEWS	Thu Dec 13 17:58:27 2018 +0100
+++ b/NEWS	Fri Dec 14 12:10:03 2018 +0100
@@ -5,13 +5,15 @@
 General
 -------
 
-* Optimization: On finds, do not read JSON (disk) if main DICOM tags (DB) are sufficient
+* Optimization: On C-FIND, avoid accessing the storage area whenever possible
+* New configuration option:
+  - "StorageAccessOnFind" to rule the access to the storage area during C-FIND
 
 Maintenance
 -----------
 
-* "/tools/create-dicom" is more tolerant to invalid specific character set
-
+* Removal of the "AllowFindSopClassesInStudy" old configuration option
+* "/tools/create-dicom" is more tolerant wrt. invalid specific character set
 
 
 Version 1.5.0 (2018-12-10)
diff -r b9f0b0c0b36f -r abe49ca61cd5 OrthancServer/OrthancFindRequestHandler.cpp
--- a/OrthancServer/OrthancFindRequestHandler.cpp	Thu Dec 13 17:58:27 2018 +0100
+++ b/OrthancServer/OrthancFindRequestHandler.cpp	Fri Dec 14 12:10:03 2018 +0100
@@ -89,82 +89,6 @@
   }
 
 
-  static void ExtractTagFromMainDicomTags(std::set<std::string>& target,
-                                          ServerIndex& index,
-                                          const DicomTag& tag,
-                                          const std::list<std::string>& resources,
-                                          ResourceType level)
-  {
-    for (std::list<std::string>::const_iterator
-           it = resources.begin(); it != resources.end(); ++it)
-    {
-      DicomMap tags;
-      if (index.GetMainDicomTags(tags, *it, level, level) &&
-          tags.HasTag(tag))
-      {
-        target.insert(tags.GetValue(tag).GetContent());
-      }
-    }
-  }
-
-
-  static bool ExtractMetadata(std::set<std::string>& target,
-                              ServerIndex& index,
-                              MetadataType metadata,
-                              const std::list<std::string>& resources)
-  {
-    for (std::list<std::string>::const_iterator
-           it = resources.begin(); it != resources.end(); ++it)
-    {
-      std::string value;
-      if (index.LookupMetadata(value, *it, metadata))
-      {
-        target.insert(value);
-      }
-      else
-      {
-        // This metadata is unavailable for some resource, give up
-        return false;
-      }
-    }
-
-    return true;
-  }  
-
-
-  static void ExtractTagFromInstancesOnDisk(std::set<std::string>& target,
-                                            ServerContext& context,
-                                            const DicomTag& tag,
-                                            const std::list<std::string>& instances)
-  {
-    // WARNING: This function is slow, as it reads the JSON file
-    // summarizing each instance of interest from the hard drive.
-
-    std::string formatted = tag.Format();
-
-    for (std::list<std::string>::const_iterator
-           it = instances.begin(); it != instances.end(); ++it)
-    {
-      Json::Value dicom;
-      context.ReadDicomAsJson(dicom, *it);
-
-      if (dicom.isMember(formatted))
-      {
-        const Json::Value& source = dicom[formatted];
-
-        if (source.type() == Json::objectValue &&
-            source.isMember("Type") &&
-            source.isMember("Value") &&
-            source["Type"].asString() == "String" &&
-            source["Value"].type() == Json::stringValue)
-        {
-          target.insert(source["Value"].asString());
-        }
-      }
-    }
-  }
-
-
   static void ComputePatientCounters(DicomMap& result,
                                      ServerIndex& index,
                                      const std::string& patient,
@@ -230,7 +154,24 @@
     if (query.HasTag(DICOM_TAG_MODALITIES_IN_STUDY))
     {
       std::set<std::string> values;
-      ExtractTagFromMainDicomTags(values, index, DICOM_TAG_MODALITY, series, ResourceType_Series);
+
+      for (std::list<std::string>::const_iterator
+             it = series.begin(); it != series.end(); ++it)
+      {
+        DicomMap tags;
+        if (index.GetMainDicomTags(tags, *it, ResourceType_Series, ResourceType_Series))
+        {
+          const DicomValue* value = tags.TestAndGetValue(DICOM_TAG_MODALITY);
+
+          if (value != NULL &&
+              !value->IsNull() &&
+              !value->IsBinary())
+          {
+            values.insert(value->GetContent());
+          }
+        }
+      }
+
       StoreSetOfStrings(result, DICOM_TAG_MODALITIES_IN_STUDY, values);
     }
 
@@ -253,27 +194,17 @@
     {
       std::set<std::string> values;
 
-      if (ExtractMetadata(values, index, MetadataType_Instance_SopClassUid, instances))
-      {
-        // The metadata "SopClassUid" is available for each of these instances
-        StoreSetOfStrings(result, DICOM_TAG_SOP_CLASSES_IN_STUDY, values);
-      }
-      else
+      for (std::list<std::string>::const_iterator
+             it = instances.begin(); it != instances.end(); ++it)
       {
-        OrthancConfiguration::ReaderLock lock;
-
-        if (lock.GetConfiguration().GetBooleanParameter("AllowFindSopClassesInStudy", false))
+        std::string value;
+        if (context.LookupOrReconstructMetadata(value, *it, MetadataType_Instance_SopClassUid))
         {
-          ExtractTagFromInstancesOnDisk(values, context, DICOM_TAG_SOP_CLASS_UID, instances);
-          StoreSetOfStrings(result, DICOM_TAG_SOP_CLASSES_IN_STUDY, values);
-        }
-        else
-        {
-          result.SetValue(DICOM_TAG_SOP_CLASSES_IN_STUDY, "", false);
-          LOG(WARNING) << "The handling of \"SOP Classes in Study\" (0008,0062) "
-                       << "in C-FIND requests is disabled";
+          values.insert(value);
         }
       }
+
+      StoreSetOfStrings(result, DICOM_TAG_SOP_CLASSES_IN_STUDY, values);
     }
   }
 
@@ -366,11 +297,22 @@
 
   static void AddAnswer(DicomFindAnswers& answers,
                         const DicomMap& mainDicomTags,
-                        const Json::Value* dicomAsJson,  // only used for sequences
+                        const Json::Value* dicomAsJson,
                         const DicomArray& query,
                         const std::list<DicomTag>& sequencesToReturn,
                         const DicomMap* counters)
   {
+    DicomMap match;
+
+    if (dicomAsJson != NULL)
+    {
+      match.FromDicomAsJson(*dicomAsJson);
+    }
+    else
+    {
+      match.Assign(mainDicomTags);
+    }
+    
     DicomMap result;
 
     for (size_t i = 0; i < query.GetSize(); i++)
@@ -384,26 +326,22 @@
       {
         // Do not include the encoding, this is handled by class ParsedDicomFile
       }
-      else if (dicomAsJson != NULL)
+      else
       {
-        std::string tag = query.GetElement(i).GetTag().Format();
-        std::string value;
-        if (dicomAsJson->isMember(tag))
+        const DicomTag& tag = query.GetElement(i).GetTag();
+        const DicomValue* value = match.TestAndGetValue(tag);
+
+        if (value != NULL &&
+            !value->IsNull() &&
+            !value->IsBinary())
         {
-          value = dicomAsJson->get(tag, Json::arrayValue).get("Value", "").asString();
-          result.SetValue(query.GetElement(i).GetTag(), value, false);
+          result.SetValue(tag, value->GetContent(), false);
         }
         else
         {
-          result.SetValue(query.GetElement(i).GetTag(), "", false);
+          result.SetValue(tag, "", false);
         }
       }
-      else
-      {
-        // Best-effort
-        // TODO
-        throw OrthancException(ErrorCode_NotImplemented);
-      }
     }
 
     if (counters != NULL)
@@ -420,11 +358,15 @@
     {
       LOG(WARNING) << "The C-FIND request does not return any DICOM tag";
     }
-    else if (sequencesToReturn.empty() ||
-             dicomAsJson == NULL)
+    else if (sequencesToReturn.empty())
     {
       answers.Add(result);
     }
+    else if (dicomAsJson == NULL)
+    {
+      LOG(WARNING) << "C-FIND query requesting a sequence, but reading JSON from disk is disabled";
+      answers.Add(result);
+    }
     else
     {
       ParsedDicomFile dicom(result);
@@ -536,48 +478,50 @@
     DicomFindAnswers&           answers_;
     ServerContext&              context_;
     ResourceType                level_;
-    const DicomMap&             filteredInput_;
+    const DicomMap&             query_;
+    DicomArray                  queryAsArray_;
     const std::list<DicomTag>&  sequencesToReturn_;
-    DicomArray                  query_;
 
   public:
     LookupVisitor(DicomFindAnswers&  answers,
                   ServerContext& context,
                   ResourceType level,
-                  const DicomMap& filteredInput,
+                  const DicomMap& query,
                   const std::list<DicomTag>& sequencesToReturn) :
       answers_(answers),
       context_(context),
       level_(level),
-      filteredInput_(filteredInput),
-      sequencesToReturn_(sequencesToReturn),
-      query_(filteredInput)
+      query_(query),
+      queryAsArray_(query),
+      sequencesToReturn_(sequencesToReturn)
     {
       answers_.SetComplete(false);
     }
 
     virtual bool IsDicomAsJsonNeeded() const
     {
-#if 1
-      return true;
-
-#else
-      // TODO
-      
       // Ask the "DICOM-as-JSON" attachment only if sequences are to
       // be returned OR if "query_" contains non-main DICOM tags!
 
-      // TODO - configuration option
-      bool findFromDatabase;
+      DicomMap withoutSpecialTags;
+      withoutSpecialTags.Assign(query_);
+
+      // Check out "ComputeCounters()"
+      withoutSpecialTags.Remove(DICOM_TAG_MODALITIES_IN_STUDY);
+      withoutSpecialTags.Remove(DICOM_TAG_NUMBER_OF_PATIENT_RELATED_INSTANCES);
+      withoutSpecialTags.Remove(DICOM_TAG_NUMBER_OF_PATIENT_RELATED_SERIES);
+      withoutSpecialTags.Remove(DICOM_TAG_NUMBER_OF_PATIENT_RELATED_STUDIES);
+      withoutSpecialTags.Remove(DICOM_TAG_NUMBER_OF_SERIES_RELATED_INSTANCES);
+      withoutSpecialTags.Remove(DICOM_TAG_NUMBER_OF_STUDY_RELATED_INSTANCES);
+      withoutSpecialTags.Remove(DICOM_TAG_NUMBER_OF_STUDY_RELATED_SERIES);
+      withoutSpecialTags.Remove(DICOM_TAG_SOP_CLASSES_IN_STUDY);
+
+      // Check out "AddAnswer()"
+      withoutSpecialTags.Remove(DICOM_TAG_SPECIFIC_CHARACTER_SET);
+      withoutSpecialTags.Remove(DICOM_TAG_QUERY_RETRIEVE_LEVEL);
       
-      {
-        // New configuration option in 1.5.1
-        OrthancConfiguration::ReaderLock lock;
-        findFromDatabase = lock.GetConfiguration().GetUnsignedIntegerParameter("FindFromDatabase", false);
-      }      
-
-      return !sequencesToReturn_.empty();
-#endif
+      return (!sequencesToReturn_.empty() ||
+              !withoutSpecialTags.HasOnlyMainDicomTags());
     }
       
     virtual void MarkAsComplete()
@@ -590,8 +534,10 @@
                        const DicomMap& mainDicomTags,
                        const Json::Value* dicomAsJson) 
     {
-      std::auto_ptr<DicomMap> counters(ComputeCounters(context_, instanceId, level_, filteredInput_));
-      AddAnswer(answers_, mainDicomTags, dicomAsJson, query_, sequencesToReturn_, counters.get());
+      std::auto_ptr<DicomMap> counters(ComputeCounters(context_, instanceId, level_, query_));
+
+      AddAnswer(answers_, mainDicomTags, dicomAsJson,
+                queryAsArray_, sequencesToReturn_, counters.get());
     }
   };
 
diff -r b9f0b0c0b36f -r abe49ca61cd5 OrthancServer/ServerContext.cpp
--- a/OrthancServer/ServerContext.cpp	Thu Dec 13 17:58:27 2018 +0100
+++ b/OrthancServer/ServerContext.cpp	Fri Dec 14 12:10:03 2018 +0100
@@ -778,6 +778,35 @@
                             size_t since,
                             size_t limit)
   {
+    LookupMode mode;
+      
+    {
+      // New configuration option in 1.5.1
+      OrthancConfiguration::ReaderLock lock;
+
+      std::string value = lock.GetConfiguration().GetStringParameter("StorageAccessOnFind", "Always");
+
+      if (value == "Always")
+      {
+        mode = LookupMode_DiskOnLookupAndAnswer;
+      }
+      else if (value == "Never")
+      {
+        mode = LookupMode_DatabaseOnly;
+      }
+      else if (value == "Answers")
+      {
+        mode = LookupMode_DiskOnAnswer;
+      }
+      else
+      {
+        throw OrthancException(ErrorCode_ParameterOutOfRange,
+                               "Configuration option \"StorageAccessOnFind\" "
+                               "should be \"Always\", \"Never\" or \"Answers\": " + value);
+      }
+    }      
+
+
     std::vector<std::string> resources, instances;
     GetIndex().FindCandidates(resources, instances, lookup);
 
@@ -788,6 +817,8 @@
     size_t countResults = 0;
     size_t skipped = 0;
     bool complete = true;
+
+    const bool isDicomAsJsonNeeded = visitor.IsDicomAsJsonNeeded();
     
     for (size_t i = 0; i < instances.size(); i++)
     {
@@ -799,11 +830,20 @@
       bool hasOnlyMainDicomTags;
       DicomMap dicom;
       
-      if (lookup.HasOnlyMainDicomTags() &&
-          GetIndex().GetAllMainDicomTags(dicom, instances[i]))
+      if (mode == LookupMode_DatabaseOnly ||
+          mode == LookupMode_DiskOnAnswer ||
+          lookup.HasOnlyMainDicomTags())
       {
         // Case (1): The main DICOM tags, as stored in the database,
         // are sufficient to look for match
+
+        if (!GetIndex().GetAllMainDicomTags(dicom, instances[i]))
+        {
+          // The instance has been removed during the execution of the
+          // lookup, ignore it
+          continue;
+        }
+        
         hasOnlyMainDicomTags = true;
       }
       else
@@ -834,8 +874,10 @@
         }
         else
         {
-          if (dicomAsJson.get() == NULL &&
-              visitor.IsDicomAsJsonNeeded())
+          if ((mode == LookupMode_DiskOnLookupAndAnswer ||
+               mode == LookupMode_DiskOnAnswer) &&
+              dicomAsJson.get() == NULL &&
+              isDicomAsJsonNeeded)
           {
             dicomAsJson.reset(new Json::Value);
             ReadDicomAsJson(*dicomAsJson, instances[i]);
@@ -870,6 +912,77 @@
   }
 
 
+  bool ServerContext::LookupOrReconstructMetadata(std::string& target,
+                                                  const std::string& publicId,
+                                                  MetadataType metadata)
+  {
+    // This is a backwards-compatibility function, that can
+    // reconstruct metadata that were not generated by an older
+    // release of Orthanc
+
+    if (metadata == MetadataType_Instance_SopClassUid ||
+        metadata == MetadataType_Instance_TransferSyntax)
+    {
+      if (index_.LookupMetadata(target, publicId, metadata))
+      {
+        return true;
+      }
+      else
+      {
+        // These metadata are mandatory in DICOM instances, and were
+        // introduced in Orthanc 1.2.0. The fact that
+        // "LookupMetadata()" has failed indicates that this database
+        // comes from an older release of Orthanc.
+        
+        DicomTag tag(0, 0);
+      
+        switch (metadata)
+        {
+          case MetadataType_Instance_SopClassUid:
+            tag = DICOM_TAG_SOP_CLASS_UID;
+            break;
+
+          case MetadataType_Instance_TransferSyntax:
+            tag = DICOM_TAG_TRANSFER_SYNTAX_UID;
+            break;
+
+          default:
+            throw OrthancException(ErrorCode_InternalError);
+        }
+      
+        Json::Value dicomAsJson;
+        ReadDicomAsJson(dicomAsJson, publicId);
+
+        DicomMap tags;
+        tags.FromDicomAsJson(dicomAsJson);
+
+        const DicomValue* value = tags.TestAndGetValue(tag);
+
+        if (value != NULL &&
+            !value->IsNull() &&
+            !value->IsBinary())
+        {
+          target = value->GetContent();
+
+          // Store for reuse
+          index_.SetMetadata(publicId, metadata, target);
+          return true;
+        }
+        else
+        {
+          // Should never happen
+          return false;
+        }
+      }
+    }
+    else
+    {
+      // No backward
+      return index_.LookupMetadata(target, publicId, metadata);
+    }
+  }
+
+
   void ServerContext::AddChildInstances(SetOfInstancesJob& job,
                                         const std::string& publicId)
   {
diff -r b9f0b0c0b36f -r abe49ca61cd5 OrthancServer/ServerContext.h
--- a/OrthancServer/ServerContext.h	Thu Dec 13 17:58:27 2018 +0100
+++ b/OrthancServer/ServerContext.h	Fri Dec 14 12:10:03 2018 +0100
@@ -64,6 +64,14 @@
   class ServerContext : private JobsRegistry::IObserver
   {
   private:
+    enum LookupMode
+    {
+      LookupMode_DatabaseOnly,
+      LookupMode_DiskOnAnswer,
+      LookupMode_DiskOnLookupAndAnswer
+    };
+
+    
     class LuaServerListener : public IServerListener
     {
     private:
@@ -340,6 +348,10 @@
                size_t since,
                size_t limit);
 
+    bool LookupOrReconstructMetadata(std::string& target,
+                                     const std::string& publicId,
+                                     MetadataType type);
+
 
     /**
      * Management of the plugins
diff -r b9f0b0c0b36f -r abe49ca61cd5 Resources/Configuration.json
--- a/Resources/Configuration.json	Thu Dec 13 17:58:27 2018 +0100
+++ b/Resources/Configuration.json	Fri Dec 14 12:10:03 2018 +0100
@@ -384,14 +384,6 @@
      }
    **/
   
-  // If set to "true", Orthanc will still handle "SOP Classes in
-  // Study" (0008,0062) in C-FIND requests, even if the "SOP Class
-  // UID" metadata is not available in the database (which is the case
-  // if the DB was previously used by Orthanc <= 1.1.0). This option
-  // is turned off by default, as it requires intensive accesses to
-  // the hard drive.
-  "AllowFindSopClassesInStudy" : false,
-
   // If set to "false", Orthanc will not load its default dictionary
   // of private tags. This might be necessary if you cannot import a
   // DICOM file encoded using the Implicit VR Endian transfer syntax,
@@ -447,5 +439,17 @@
   // The least recently used archives get deleted as new archives are
   // generated. This option was introduced in Orthanc 1.5.0, and has
   // no effect on the synchronous generation of archives.
-  "MediaArchiveSize" : 1
+  "MediaArchiveSize" : 1,
+
+  // Performance setting to specify how Orthanc accesses the storage
+  // area during C-FIND. Three modes are available: (1) "Always"
+  // allows Orthanc to read the storage area as soon as it needs an
+  // information that is not present in its database (slowest mode),
+  // (2) "Never" prevents Orthanc from accessing the storage area, and
+  // makes it uses exclusively its database (fastest mode), and (3)
+  // "Answers" allows Orthanc to read the storage area to generate its
+  // answers, but not to filter the DICOM resources (balance between
+  // the two modes). By default, the mode is "Always", which
+  // corresponds to the behavior of Orthanc <= 1.5.0.
+  "StorageAccessOnFind" : "Always"
 }
diff -r b9f0b0c0b36f -r abe49ca61cd5 UnitTestsSources/DicomMapTests.cpp
--- a/UnitTestsSources/DicomMapTests.cpp	Thu Dec 13 17:58:27 2018 +0100
+++ b/UnitTestsSources/DicomMapTests.cpp	Fri Dec 14 12:10:03 2018 +0100
@@ -511,6 +511,7 @@
 {
   DicomMap b;
   b.SetValue(DICOM_TAG_PATIENT_NAME, "E", false);
+  ASSERT_TRUE(b.HasOnlyMainDicomTags());
 
   {
     DicomMap a;
@@ -519,6 +520,7 @@
     a.SetValue(DICOM_TAG_SERIES_DESCRIPTION, "C", false);
     a.SetValue(DICOM_TAG_NUMBER_OF_FRAMES, "D", false);
     a.SetValue(DICOM_TAG_SLICE_THICKNESS, "F", false);
+    ASSERT_FALSE(a.HasOnlyMainDicomTags());
     b.ExtractMainDicomTags(a);
   }
 
@@ -528,6 +530,7 @@
   ASSERT_EQ("C", b.GetValue(DICOM_TAG_SERIES_DESCRIPTION).GetContent());
   ASSERT_EQ("D", b.GetValue(DICOM_TAG_NUMBER_OF_FRAMES).GetContent());
   ASSERT_FALSE(b.HasTag(DICOM_TAG_SLICE_THICKNESS));
+  ASSERT_TRUE(b.HasOnlyMainDicomTags());
 
   b.SetValue(DICOM_TAG_PATIENT_NAME, "G", false);
 
@@ -535,6 +538,7 @@
     DicomMap a;
     a.SetValue(DICOM_TAG_PATIENT_NAME, "A", false);
     a.SetValue(DICOM_TAG_SLICE_THICKNESS, "F", false);
+    ASSERT_FALSE(a.HasOnlyMainDicomTags());
     b.Merge(a);
   }
 
@@ -544,4 +548,5 @@
   ASSERT_EQ("C", b.GetValue(DICOM_TAG_SERIES_DESCRIPTION).GetContent());
   ASSERT_EQ("D", b.GetValue(DICOM_TAG_NUMBER_OF_FRAMES).GetContent());
   ASSERT_EQ("F", b.GetValue(DICOM_TAG_SLICE_THICKNESS).GetContent());
+  ASSERT_FALSE(b.HasOnlyMainDicomTags());
 }