# HG changeset patch # User Sebastien Jodogne # Date 1714748522 -7200 # Node ID fc3914c07dd38d5b891689b50f699c193af9f17a # Parent 74cc31c8db2b32366926f2023cffb1e39a72e534 refactoring FindResponse diff -r 74cc31c8db2b -r fc3914c07dd3 OrthancServer/Sources/Database/Compatibility/GenericFind.cpp --- a/OrthancServer/Sources/Database/Compatibility/GenericFind.cpp Sat Apr 27 22:15:37 2024 +0200 +++ b/OrthancServer/Sources/Database/Compatibility/GenericFind.cpp Fri May 03 17:02:02 2024 +0200 @@ -59,10 +59,7 @@ for (std::list::const_iterator it = ids.begin(); it != ids.end(); ++it) { - OrthancIdentifiers identifiers; - identifiers.SetLevel(request.GetLevel(), *it); - - response.Add(new FindResponse::Item(identifiers)); + response.Add(new FindResponse::Item(request.GetLevel(), *it)); } } else @@ -79,10 +76,14 @@ { const FindResponse::Item& item = response.GetItem(i); - if (request.HasResponseContent(FindRequest::ResponseContent_MainDicomTags) - && !item.HasDicomMap()) + if (request.HasResponseContent(FindRequest::ResponseContent_MainDicomTags)) { - throw OrthancException(ErrorCode_InternalError); + DicomMap tmp; + item.GetDicomTagsAtLevel(tmp, request.GetLevel()); + if (tmp.GetSize() == 0) + { + throw OrthancException(ErrorCode_InternalError); + } } // TODO: other sanity checks diff -r 74cc31c8db2b -r fc3914c07dd3 OrthancServer/Sources/Database/FindResponse.cpp --- a/OrthancServer/Sources/Database/FindResponse.cpp Sat Apr 27 22:15:37 2024 +0200 +++ b/OrthancServer/Sources/Database/FindResponse.cpp Fri May 03 17:02:02 2024 +0200 @@ -30,101 +30,189 @@ namespace Orthanc { - static void ExtractOrthancIdentifiers(OrthancIdentifiers& identifiers, - ResourceType level, - const DicomMap& dicom) + class FindResponse::DicomTagsAtLevel::DicomValue : public boost::noncopyable + { + public: + enum ValueType + { + ValueType_String, + ValueType_Null + }; + + private: + ValueType type_; + std::string value_; + + public: + DicomValue(ValueType type, + const std::string& value) : + type_(type), + value_(value) + { + } + + ValueType GetType() const + { + return type_; + } + + const std::string& GetValue() const + { + switch (type_) + { + case ValueType_Null: + throw OrthancException(ErrorCode_BadSequenceOfCalls); + + case ValueType_String: + return value_; + + default: + throw OrthancException(ErrorCode_ParameterOutOfRange); + } + } + }; + + + FindResponse::DicomTagsAtLevel::~DicomTagsAtLevel() + { + for (Content::iterator it = content_.begin(); it != content_.end(); ++it) + { + assert(it->second != NULL); + delete it->second; + } + } + + + void FindResponse::DicomTagsAtLevel::AddNullValue(uint16_t group, + uint16_t element) + { + const DicomTag tag(group, element); + + if (content_.find(tag) == content_.end()) + { + content_[tag] = new DicomValue(DicomValue::ValueType_Null, ""); + } + else + { + throw OrthancException(ErrorCode_BadSequenceOfCalls); + } + } + + + void FindResponse::DicomTagsAtLevel::AddStringValue(uint16_t group, + uint16_t element, + const std::string& value) + { + const DicomTag tag(group, element); + + if (content_.find(tag) == content_.end()) + { + content_[tag] = new DicomValue(DicomValue::ValueType_String, value); + } + else + { + throw OrthancException(ErrorCode_BadSequenceOfCalls); + } + } + + + void FindResponse::DicomTagsAtLevel::Fill(DicomMap& target) const + { + for (Content::const_iterator it = content_.begin(); it != content_.end(); ++it) + { + assert(it->second != NULL); + + switch (it->second->GetType()) + { + case DicomValue::ValueType_String: + target.SetValue(it->first, it->second->GetValue(), false /* not binary */); + break; + + case DicomValue::ValueType_Null: + target.SetNullValue(it->first); + break; + + default: + throw OrthancException(ErrorCode_InternalError); + } + } + } + + + void FindResponse::ChildrenAtLevel::AddIdentifier(const std::string& identifier) + { + if (identifiers_.find(identifier) == identifiers_.end()) + { + identifiers_.insert(identifier); + } + else + { + throw OrthancException(ErrorCode_BadSequenceOfCalls); + } + } + + + FindResponse::DicomTagsAtLevel& FindResponse::Item::GetDicomTagsAtLevel(ResourceType level) { switch (level) { case ResourceType_Patient: - { - std::string patientId; - if (!dicom.LookupStringValue(patientId, Orthanc::DICOM_TAG_PATIENT_ID, false)) - { - throw OrthancException(ErrorCode_ParameterOutOfRange); - } - else - { - DicomInstanceHasher hasher(patientId, "", "", ""); - identifiers.SetPatientId(hasher.HashPatient()); - } - break; - } + return patientTags_; case ResourceType_Study: - { - std::string patientId, studyInstanceUid; - if (!dicom.LookupStringValue(patientId, Orthanc::DICOM_TAG_PATIENT_ID, false) || - !dicom.LookupStringValue(studyInstanceUid, Orthanc::DICOM_TAG_STUDY_INSTANCE_UID, false)) - { - throw OrthancException(ErrorCode_ParameterOutOfRange); - } - else - { - DicomInstanceHasher hasher(patientId, studyInstanceUid, "", ""); - identifiers.SetPatientId(hasher.HashPatient()); - identifiers.SetStudyId(hasher.HashStudy()); - } - break; - } + return studyTags_; case ResourceType_Series: - { - std::string patientId, studyInstanceUid, seriesInstanceUid; - if (!dicom.LookupStringValue(patientId, Orthanc::DICOM_TAG_PATIENT_ID, false) || - !dicom.LookupStringValue(studyInstanceUid, Orthanc::DICOM_TAG_STUDY_INSTANCE_UID, false) || - !dicom.LookupStringValue(seriesInstanceUid, Orthanc::DICOM_TAG_SERIES_INSTANCE_UID, false)) - { - throw OrthancException(ErrorCode_ParameterOutOfRange); - } - else - { - DicomInstanceHasher hasher(patientId, studyInstanceUid, seriesInstanceUid, ""); - identifiers.SetPatientId(hasher.HashPatient()); - identifiers.SetStudyId(hasher.HashStudy()); - identifiers.SetSeriesId(hasher.HashSeries()); - } - break; - } + return seriesTags_; case ResourceType_Instance: - { - std::string patientId, studyInstanceUid, seriesInstanceUid, sopInstanceUid; - if (!dicom.LookupStringValue(patientId, Orthanc::DICOM_TAG_PATIENT_ID, false) || - !dicom.LookupStringValue(studyInstanceUid, Orthanc::DICOM_TAG_STUDY_INSTANCE_UID, false) || - !dicom.LookupStringValue(seriesInstanceUid, Orthanc::DICOM_TAG_SERIES_INSTANCE_UID, false) || - !dicom.LookupStringValue(sopInstanceUid, Orthanc::DICOM_TAG_SOP_INSTANCE_UID, false)) - { - throw OrthancException(ErrorCode_ParameterOutOfRange); - } - else - { - DicomInstanceHasher hasher(patientId, studyInstanceUid, seriesInstanceUid, sopInstanceUid); - identifiers.SetPatientId(hasher.HashPatient()); - identifiers.SetStudyId(hasher.HashStudy()); - identifiers.SetSeriesId(hasher.HashSeries()); - identifiers.SetInstanceId(hasher.HashInstance()); - } - break; - } + return instanceTags_; default: - throw OrthancException(ErrorCode_NotImplemented); + throw OrthancException(ErrorCode_ParameterOutOfRange); } } - FindResponse::Item::Item(ResourceType level, - DicomMap* dicomMap /* takes ownership */) : - dicomMap_(dicomMap) + FindResponse::ChildrenAtLevel& FindResponse::Item::GetChildrenAtLevel(ResourceType level) { - if (dicomMap == NULL) + switch (level) { - throw OrthancException(ErrorCode_NullPointer); - } - else - { - ExtractOrthancIdentifiers(identifiers_, level, *dicomMap); + case ResourceType_Study: + if (level_ == ResourceType_Patient) + { + return childrenStudies_; + } + else + { + throw OrthancException(ErrorCode_BadParameterType); + } + + case ResourceType_Series: + if (level_ == ResourceType_Patient || + level_ == ResourceType_Study) + { + return childrenSeries_; + } + else + { + throw OrthancException(ErrorCode_BadParameterType); + } + + case ResourceType_Instance: + if (level_ == ResourceType_Patient || + level_ == ResourceType_Study || + level_ == ResourceType_Series) + { + return childrenInstances_; + } + else + { + throw OrthancException(ErrorCode_BadParameterType); + } + + default: + throw OrthancException(ErrorCode_ParameterOutOfRange); } } @@ -170,16 +258,90 @@ } } + const std::string& FindResponse::Item::GetParentIdentifier() const + { + if (level_ == ResourceType_Patient) + { + throw OrthancException(ErrorCode_BadParameterType); + } + else if (HasParentIdentifier()) + { + return *parentIdentifier_; + } + else + { + throw OrthancException(ErrorCode_BadSequenceOfCalls); + } + } - const DicomMap& FindResponse::Item::GetDicomMap() const + + void FindResponse::Item::SetParentIdentifier(const std::string& id) { - if (dicomMap_.get() == NULL) + if (level_ == ResourceType_Patient) + { + throw OrthancException(ErrorCode_BadParameterType); + } + else if (HasParentIdentifier()) { throw OrthancException(ErrorCode_BadSequenceOfCalls); } else { - return *dicomMap_; + parentIdentifier_.reset(new std::string(id)); + } + } + + + bool FindResponse::Item::HasParentIdentifier() const + { + if (level_ == ResourceType_Patient) + { + throw OrthancException(ErrorCode_BadParameterType); + } + else + { + return parentIdentifier_.get() != NULL; + } + } + + + void FindResponse::Item::AddLabel(const std::string& label) + { + if (labels_.find(label) == labels_.end()) + { + labels_.insert(label); + } + else + { + throw OrthancException(ErrorCode_BadSequenceOfCalls); + } + } + + + void FindResponse::Item::AddAttachment(const FileInfo& attachment) + { + if (attachments_.find(attachment.GetContentType()) == attachments_.end()) + { + attachments_[attachment.GetContentType()] = attachment; + } + else + { + throw OrthancException(ErrorCode_BadSequenceOfCalls); + } + } + + + bool FindResponse::Item::LookupAttachment(FileInfo& target, FileContentType type) const + { + std::map::const_iterator it = attachments_.find(type); + if (it != attachments_.end()) + { + target = it->second; + return true; + } + else + { + return false; } } @@ -196,13 +358,25 @@ void FindResponse::Add(Item* item /* takes ownership */) { + std::unique_ptr protection(item); + if (item == NULL) { throw OrthancException(ErrorCode_NullPointer); } else { - items_.push_back(item); + const std::string& id = item->GetIdentifier(); + + if (index_.find(id) == index_.end()) + { + items_.push_back(protection.release()); + index_[id] = item; + } + else + { + throw OrthancException(ErrorCode_BadSequenceOfCalls, "This resource has already been added: " + id); + } } } @@ -220,20 +394,19 @@ } } - void FindResponse::Item::AddDicomTag(uint16_t group, uint16_t element, const std::string& value, bool isBinary) + + const FindResponse::Item* FindResponse::LookupItem(const std::string& id) const { - if (dicomMap_.get() == NULL) + Index::const_iterator found = index_.find(id); + + if (found == index_.end()) { - dicomMap_.reset(new DicomMap()); + return NULL; } - - dicomMap_->SetValue(group, element, value, isBinary); + else + { + assert(found->second != NULL); + return found->second; + } } - - void FindResponse::Item::AddChild(const std::string& childId) - { - children_.push_back(childId); - } - - } diff -r 74cc31c8db2b -r fc3914c07dd3 OrthancServer/Sources/Database/FindResponse.h --- a/OrthancServer/Sources/Database/FindResponse.h Sat Apr 27 22:15:37 2024 +0200 +++ b/OrthancServer/Sources/Database/FindResponse.h Fri May 03 17:02:02 2024 +0200 @@ -40,75 +40,135 @@ { class FindResponse : public boost::noncopyable { - public: - - // TODO-FIND: does it actually make sense to retrieve revisions for metadata and attachments ? - class StringWithRevision + private: + class DicomTagsAtLevel : public boost::noncopyable { private: - std::string value_; - int64_t revision_; + class DicomValue; + + typedef std::map Content; + + Content content_; + public: - StringWithRevision(const std::string& value, - int64_t revision) : - value_(value), - revision_(revision) - { - } + ~DicomTagsAtLevel(); + + void AddNullValue(uint16_t group, + uint16_t element); + + void AddStringValue(uint16_t group, + uint16_t element, + const std::string& value); - StringWithRevision(const StringWithRevision& other) : - value_(other.value_), - revision_(other.revision_) + void Fill(DicomMap& target) const; + }; + + + class ChildrenAtLevel : public boost::noncopyable + { + private: + std::set identifiers_; + + public: + void AddIdentifier(const std::string& identifier); + + const std::set& GetIdentifiers() const { - } - - StringWithRevision() : - revision_(-1) - { - } - - const std::string& GetValue() const - { - return value_; - } - - int64_t GetRevision() const - { - return revision_; + return identifiers_; } }; + public: class Item : public boost::noncopyable { private: - OrthancIdentifiers identifiers_; - std::unique_ptr dicomMap_; - std::list children_; - std::string childInstanceId_; + ResourceType level_; + std::string identifier_; + std::unique_ptr parentIdentifier_; + DicomTagsAtLevel patientTags_; + DicomTagsAtLevel studyTags_; + DicomTagsAtLevel seriesTags_; + DicomTagsAtLevel instanceTags_; + ChildrenAtLevel childrenStudies_; + ChildrenAtLevel childrenSeries_; + ChildrenAtLevel childrenInstances_; std::set labels_; std::map metadata_; std::map attachments_; + DicomTagsAtLevel& GetDicomTagsAtLevel(ResourceType level); + + ChildrenAtLevel& GetChildrenAtLevel(ResourceType level); + public: - explicit Item(const OrthancIdentifiers& identifiers) : - identifiers_(identifiers) + explicit Item(ResourceType level, + const std::string& identifier) : + level_(level), + identifier_(identifier) { } - Item(ResourceType level, - DicomMap* dicomMap /* takes ownership */); + ResourceType GetLevel() const + { + return level_; + } + + const std::string& GetIdentifier() const + { + return identifier_; + } - const OrthancIdentifiers& GetIdentifiers() const + const std::string& GetParentIdentifier() const; + + void SetParentIdentifier(const std::string& id); + + bool HasParentIdentifier() const; + + void AddStringDicomTag(ResourceType level, + uint16_t group, + uint16_t element, + const std::string& value) { - return identifiers_; + GetDicomTagsAtLevel(level).AddStringValue(group, element, value); } - void AddDicomTag(uint16_t group, uint16_t element, const std::string& value, bool isBinary); + // The "Null" value could be used in the future to indicate a + // value that is not available, typically a new "ExtraMainDicomTag" + void AddNullDicomTag(ResourceType level, + uint16_t group, + uint16_t element, + const std::string& value) + { + GetDicomTagsAtLevel(level).AddNullValue(group, element); + } + + void GetDicomTagsAtLevel(DicomMap& target, + ResourceType level) const + { + const_cast(*this).GetDicomTagsAtLevel(level).Fill(target); + } + + void AddChildIdentifier(ResourceType level, + const std::string& childId) + { + GetChildrenAtLevel(level).AddIdentifier(childId); + } + + const std::set& GetChildrenIdentifiers(ResourceType level) const + { + return const_cast(*this).GetChildrenAtLevel(level).GetIdentifiers(); + } + + void AddLabel(const std::string& label); + + const std::set& GetLabels() const + { + return labels_; + } void AddMetadata(MetadataType metadata, const std::string& value); - //int64_t revision); const std::map& GetMetadata() const { @@ -120,68 +180,22 @@ return metadata_.find(metadata) != metadata_.end(); } - bool LookupMetadata(std::string& value, /* int64_t revision, */ + bool LookupMetadata(std::string& value, MetadataType metadata) const; void ListMetadata(std::set& metadata) const; - bool HasDicomMap() const - { - return dicomMap_.get() != NULL; - } - - const DicomMap& GetDicomMap() const; - - void AddChild(const std::string& childId); - - const std::list& GetChildren() const - { - return children_; - } - - void AddLabel(const std::string& label) - { - labels_.insert(label); - } - - const std::set& GetLabels() const - { - return labels_; - } + void AddAttachment(const FileInfo& attachment); - void AddAttachment(const FileInfo& attachment) - { - attachments_[attachment.GetContentType()] = attachment; - } - - const std::map& GetAttachments() const - { - return attachments_; - } - - bool LookupAttachment(FileInfo& target, FileContentType type) const - { - std::map::const_iterator it = attachments_.find(type); - if (it != attachments_.end()) - { - target = it->second; - return true; - } - - return false; - } - - void SetIdentifier(ResourceType level, - const std::string& id) - { - identifiers_.SetLevel(level, id); - } - - // TODO-FIND: add other getters and setters + bool LookupAttachment(FileInfo& target, + FileContentType type) const; }; private: + typedef std::map Index; + std::deque items_; + Index index_; public: ~FindResponse(); @@ -194,5 +208,7 @@ } const Item& GetItem(size_t index) const; + + const Item* LookupItem(const std::string& id) const; }; } diff -r 74cc31c8db2b -r fc3914c07dd3 OrthancServer/Sources/Database/SQLiteDatabaseWrapper.cpp --- a/OrthancServer/Sources/Database/SQLiteDatabaseWrapper.cpp Sat Apr 27 22:15:37 2024 +0200 +++ b/OrthancServer/Sources/Database/SQLiteDatabaseWrapper.cpp Fri May 03 17:02:02 2024 +0200 @@ -1173,11 +1173,7 @@ while (statement.Step()) { - OrthancIdentifiers id; - id.SetLevel(request.GetLevel(), statement.ColumnString(0)); - - FindResponse::Item* item = new FindResponse::Item(id); - response.Add(item); + response.Add(new FindResponse::Item(request.GetLevel(), statement.ColumnString(0))); } } else @@ -1201,10 +1197,7 @@ { const std::string resourceId = statement.ColumnString(0); - OrthancIdentifiers id; - id.SetLevel(request.GetLevel(), resourceId); - - FindResponse::Item* item = new FindResponse::Item(id); + FindResponse::Item* item = new FindResponse::Item(request.GetLevel(), resourceId); items[resourceId] = item; response.Add(item); } @@ -1222,9 +1215,10 @@ while (statement.Step()) { const std::string& resourceId = statement.ColumnString(0); - items[resourceId]->AddDicomTag(statement.ColumnInt(1), - statement.ColumnInt(2), - statement.ColumnString(3), false); + items[resourceId]->AddStringDicomTag(request.GetLevel(), + statement.ColumnInt(1), + statement.ColumnInt(2), + statement.ColumnString(3)); } } @@ -1240,7 +1234,7 @@ while (statement.Step()) { const std::string& resourceId = statement.ColumnString(0); - items[resourceId]->AddChild(statement.ColumnString(1)); + items[resourceId]->AddChildIdentifier(GetChildResourceType(request.GetLevel()), statement.ColumnString(1)); } } @@ -1255,7 +1249,8 @@ while (statement.Step()) { const std::string& resourceId = statement.ColumnString(0); - items[resourceId]->SetIdentifier(GetParentResourceType(request.GetLevel()), statement.ColumnString(1)); + const std::string& parentId = statement.ColumnString(1); + items[resourceId]->SetParentIdentifier(parentId); } } diff -r 74cc31c8db2b -r fc3914c07dd3 OrthancServer/Sources/Database/StatelessDatabaseOperations.cpp --- a/OrthancServer/Sources/Database/StatelessDatabaseOperations.cpp Sat Apr 27 22:15:37 2024 +0200 +++ b/OrthancServer/Sources/Database/StatelessDatabaseOperations.cpp Fri May 03 17:02:02 2024 +0200 @@ -3865,7 +3865,7 @@ // But, finally, we might just get rid of ExpandedResource and replace it by FindResponse ExpandedResource::ExpandedResource(const FindRequest& request, const FindResponse::Item& item) : - id_(item.GetIdentifiers().GetLevel(request.GetLevel())), + id_(item.GetIdentifier()), level_(request.GetLevel()), isStable_(false), expectedNumberOfInstances_(0), @@ -3874,17 +3874,21 @@ { if (request.HasResponseContent(FindRequest::ResponseContent_MainDicomTags)) { - tags_.Assign(item.GetDicomMap()); + item.GetDicomTagsAtLevel(tags_, request.GetLevel()); } if (request.HasResponseContent(FindRequest::ResponseContent_Children)) { - childrenIds_ = item.GetChildren(); + const std::set& s = item.GetChildrenIdentifiers(GetChildResourceType(request.GetLevel())); + for (std::set::const_iterator it = s.begin(); it != s.end(); ++it) + { + childrenIds_.push_back(*it); + } } if (request.HasResponseContent(FindRequest::ResponseContent_Parent)) { - parentId_ = item.GetIdentifiers().GetLevel(GetParentResourceType(request.GetLevel())); + parentId_ = item.GetParentIdentifier(); } if (request.HasResponseContent(FindRequest::ResponseContent_Metadata)) diff -r 74cc31c8db2b -r fc3914c07dd3 OrthancServer/Sources/OrthancRestApi/OrthancRestResources.cpp --- a/OrthancServer/Sources/OrthancRestApi/OrthancRestResources.cpp Sat Apr 27 22:15:37 2024 +0200 +++ b/OrthancServer/Sources/OrthancRestApi/OrthancRestResources.cpp Fri May 03 17:02:02 2024 +0200 @@ -315,8 +315,7 @@ { for (size_t i = 0; i < response.GetSize(); i++) { - std::string resourceId = response.GetItem(i).GetIdentifiers().GetLevel(resourceType); - answer.append(resourceId); + answer.append(response.GetItem(i).GetIdentifier()); } } else