# HG changeset patch # User Sebastien Jodogne # Date 1537970142 -7200 # Node ID ff0ed5ea9e4e2b605ca0d094f665cf9e2f2d016a # Parent bf019ee38498113843780437ec0f561b2a06120f trailing step in SetOfInstancesJob diff -r bf019ee38498 -r ff0ed5ea9e4e Core/JobsEngine/SetOfInstancesJob.cpp --- a/Core/JobsEngine/SetOfInstancesJob.cpp Wed Sep 26 09:50:30 2018 +0200 +++ b/Core/JobsEngine/SetOfInstancesJob.cpp Wed Sep 26 15:55:42 2018 +0200 @@ -39,7 +39,8 @@ namespace Orthanc { - SetOfInstancesJob::SetOfInstancesJob() : + SetOfInstancesJob::SetOfInstancesJob(bool hasTrailingStep) : + hasTrailingStep_(hasTrailingStep), started_(false), permissive_(false), position_(0) @@ -60,6 +61,19 @@ } + size_t SetOfInstancesJob::GetStepsCount() const + { + if (HasTrailingStep()) + { + return instances_.size() + 1; + } + else + { + return instances_.size(); + } + } + + void SetOfInstancesJob::AddInstance(const std::string& instance) { if (started_) @@ -75,7 +89,7 @@ void SetOfInstancesJob::SetPermissive(bool permissive) { - if (IsStarted()) + if (started_) { throw OrthancException(ErrorCode_BadSequenceOfCalls); } @@ -102,21 +116,23 @@ float SetOfInstancesJob::GetProgress() { - if (instances_.size() == 0) + const size_t steps = GetStepsCount(); + + if (steps == 0) { return 0; } else { return (static_cast(position_) / - static_cast(instances_.size())); + static_cast(steps)); } } const std::string& SetOfInstancesJob::GetInstance(size_t index) const { - if (index > instances_.size()) + if (index >= instances_.size()) { throw OrthancException(ErrorCode_ParameterOutOfRange); } @@ -134,27 +150,38 @@ throw OrthancException(ErrorCode_InternalError); } - if (instances_.empty() && + const size_t steps = GetStepsCount(); + + if (steps == 0 && position_ == 0) { - // No instance to handle, we're done + // Nothing to handle (no instance, nor trailing step): We're done position_ = 1; return JobStepResult::Success(); } - - if (position_ >= instances_.size()) + + if (position_ >= steps) { // Already done throw OrthancException(ErrorCode_BadSequenceOfCalls); } - const std::string currentInstance = instances_[position_]; + bool isTrailingStep = (hasTrailingStep_ && + position_ + 1 == steps); bool ok; try { - ok = HandleInstance(currentInstance); + if (isTrailingStep) + { + ok = HandleTrailingStep(); + } + else + { + // Not at the trailing step: Handle the current instance + ok = HandleInstance(instances_[position_]); + } if (!ok && !permissive_) { @@ -173,14 +200,15 @@ } } - if (!ok) + if (!ok && + !isTrailingStep) { - failedInstances_.insert(currentInstance); + failedInstances_.insert(instances_[position_]); } position_ += 1; - if (position_ == instances_.size()) + if (position_ == steps) { // We're done return JobStepResult::Success(); @@ -191,10 +219,20 @@ } } - + + + static const char* KEY_DESCRIPTION = "Description"; + static const char* KEY_PERMISSIVE = "Permissive"; + static const char* KEY_POSITION = "Position"; + static const char* KEY_TYPE = "Type"; + static const char* KEY_INSTANCES = "Instances"; + static const char* KEY_FAILED_INSTANCES = "FailedInstances"; + static const char* KEY_TRAILING_STEP = "TrailingStep"; + + void SetOfInstancesJob::GetPublicContent(Json::Value& value) { - value["Description"] = GetDescription(); + value[KEY_DESCRIPTION] = GetDescription(); value["InstancesCount"] = static_cast(instances_.size()); value["FailedInstancesCount"] = static_cast(failedInstances_.size()); } @@ -206,14 +244,15 @@ std::string type; GetJobType(type); - value["Type"] = type; + value[KEY_TYPE] = type; + + value[KEY_PERMISSIVE] = permissive_; + value[KEY_POSITION] = static_cast(position_); + value[KEY_DESCRIPTION] = description_; + value[KEY_TRAILING_STEP] = hasTrailingStep_; - value["Permissive"] = permissive_; - value["Position"] = static_cast(position_); - value["Description"] = description_; - - SerializationToolbox::WriteArrayOfStrings(value, instances_, "Instances"); - SerializationToolbox::WriteSetOfStrings(value, failedInstances_, "FailedInstances"); + SerializationToolbox::WriteArrayOfStrings(value, instances_, KEY_INSTANCES); + SerializationToolbox::WriteSetOfStrings(value, failedInstances_, KEY_FAILED_INSTANCES); return true; } @@ -221,14 +260,24 @@ SetOfInstancesJob::SetOfInstancesJob(const Json::Value& value) : started_(false), - permissive_(SerializationToolbox::ReadBoolean(value, "Permissive")), - position_(SerializationToolbox::ReadUnsignedInteger(value, "Position")), - description_(SerializationToolbox::ReadString(value, "Description")) + permissive_(SerializationToolbox::ReadBoolean(value, KEY_PERMISSIVE)), + position_(SerializationToolbox::ReadUnsignedInteger(value, KEY_POSITION)), + description_(SerializationToolbox::ReadString(value, KEY_DESCRIPTION)) { - SerializationToolbox::ReadArrayOfStrings(instances_, value, "Instances"); - SerializationToolbox::ReadSetOfStrings(failedInstances_, value, "FailedInstances"); + SerializationToolbox::ReadArrayOfStrings(instances_, value, KEY_INSTANCES); + SerializationToolbox::ReadSetOfStrings(failedInstances_, value, KEY_FAILED_INSTANCES); - if (position_ > instances_.size()) + if (value.isMember(KEY_TRAILING_STEP)) + { + hasTrailingStep_ = SerializationToolbox::ReadBoolean(value, KEY_TRAILING_STEP); + } + else + { + // Backward compatibility with Orthanc <= 1.4.2 + hasTrailingStep_ = false; + } + + if (position_ > GetStepsCount() + 1) { throw OrthancException(ErrorCode_BadFileFormat); } diff -r bf019ee38498 -r ff0ed5ea9e4e Core/JobsEngine/SetOfInstancesJob.h --- a/Core/JobsEngine/SetOfInstancesJob.h Wed Sep 26 09:50:30 2018 +0200 +++ b/Core/JobsEngine/SetOfInstancesJob.h Wed Sep 26 15:55:42 2018 +0200 @@ -42,6 +42,7 @@ class SetOfInstancesJob : public IJob { private: + bool hasTrailingStep_; bool started_; std::vector instances_; bool permissive_; @@ -52,11 +53,18 @@ protected: virtual bool HandleInstance(const std::string& instance) = 0; + virtual bool HandleTrailingStep() = 0; + public: - SetOfInstancesJob(); + SetOfInstancesJob(bool hasTrailingStep); SetOfInstancesJob(const Json::Value& s); // Unserialization + bool HasTrailingStep() const + { + return hasTrailingStep_; + } + size_t GetPosition() const { return position_; @@ -79,6 +87,8 @@ return instances_.size(); } + size_t GetStepsCount() const; + void AddInstance(const std::string& instance); bool IsPermissive() const diff -r bf019ee38498 -r ff0ed5ea9e4e OrthancServer/ServerJobs/DicomModalityStoreJob.cpp --- a/OrthancServer/ServerJobs/DicomModalityStoreJob.cpp Wed Sep 26 09:50:30 2018 +0200 +++ b/OrthancServer/ServerJobs/DicomModalityStoreJob.cpp Wed Sep 26 15:55:42 2018 +0200 @@ -85,7 +85,14 @@ } + bool DicomModalityStoreJob::HandleTrailingStep() + { + throw OrthancException(ErrorCode_InternalError); + } + + DicomModalityStoreJob::DicomModalityStoreJob(ServerContext& context) : + SetOfInstancesJob(false /* no trailing step */), context_(context), localAet_("ORTHANC"), moveOriginatorId_(0) // By default, not a C-MOVE diff -r bf019ee38498 -r ff0ed5ea9e4e OrthancServer/ServerJobs/DicomModalityStoreJob.h --- a/OrthancServer/ServerJobs/DicomModalityStoreJob.h Wed Sep 26 09:50:30 2018 +0200 +++ b/OrthancServer/ServerJobs/DicomModalityStoreJob.h Wed Sep 26 15:55:42 2018 +0200 @@ -55,6 +55,8 @@ protected: virtual bool HandleInstance(const std::string& instance); + virtual bool HandleTrailingStep(); + public: DicomModalityStoreJob(ServerContext& context); diff -r bf019ee38498 -r ff0ed5ea9e4e OrthancServer/ServerJobs/OrthancPeerStoreJob.cpp --- a/OrthancServer/ServerJobs/OrthancPeerStoreJob.cpp Wed Sep 26 09:50:30 2018 +0200 +++ b/OrthancServer/ServerJobs/OrthancPeerStoreJob.cpp Wed Sep 26 15:55:42 2018 +0200 @@ -74,6 +74,12 @@ } + bool OrthancPeerStoreJob::HandleTrailingStep() + { + throw OrthancException(ErrorCode_InternalError); + } + + void OrthancPeerStoreJob::SetPeer(const WebServiceParameters& peer) { if (IsStarted()) diff -r bf019ee38498 -r ff0ed5ea9e4e OrthancServer/ServerJobs/OrthancPeerStoreJob.h --- a/OrthancServer/ServerJobs/OrthancPeerStoreJob.h Wed Sep 26 09:50:30 2018 +0200 +++ b/OrthancServer/ServerJobs/OrthancPeerStoreJob.h Wed Sep 26 15:55:42 2018 +0200 @@ -51,8 +51,11 @@ protected: virtual bool HandleInstance(const std::string& instance); + virtual bool HandleTrailingStep(); + public: OrthancPeerStoreJob(ServerContext& context) : + SetOfInstancesJob(false /* no trailing step */), context_(context) { } diff -r bf019ee38498 -r ff0ed5ea9e4e OrthancServer/ServerJobs/ResourceModificationJob.cpp --- a/OrthancServer/ServerJobs/ResourceModificationJob.cpp Wed Sep 26 09:50:30 2018 +0200 +++ b/OrthancServer/ServerJobs/ResourceModificationJob.cpp Wed Sep 26 15:55:42 2018 +0200 @@ -215,9 +215,14 @@ return true; } - + bool ResourceModificationJob::HandleTrailingStep() + { + throw OrthancException(ErrorCode_InternalError); + } + + void ResourceModificationJob::SetModification(DicomModification* modification, bool isAnonymization) { diff -r bf019ee38498 -r ff0ed5ea9e4e OrthancServer/ServerJobs/ResourceModificationJob.h --- a/OrthancServer/ServerJobs/ResourceModificationJob.h Wed Sep 26 09:50:30 2018 +0200 +++ b/OrthancServer/ServerJobs/ResourceModificationJob.h Wed Sep 26 15:55:42 2018 +0200 @@ -75,8 +75,11 @@ protected: virtual bool HandleInstance(const std::string& instance); + virtual bool HandleTrailingStep(); + public: ResourceModificationJob(ServerContext& context) : + SetOfInstancesJob(false /* no trailing step */), context_(context), isAnonymization_(false) { diff -r bf019ee38498 -r ff0ed5ea9e4e UnitTestsSources/MultiThreadingTests.cpp --- a/UnitTestsSources/MultiThreadingTests.cpp Wed Sep 26 09:50:30 2018 +0200 +++ b/UnitTestsSources/MultiThreadingTests.cpp Wed Sep 26 15:55:42 2018 +0200 @@ -146,20 +146,58 @@ class DummyInstancesJob : public SetOfInstancesJob { + private: + bool trailingStepDone_; + protected: virtual bool HandleInstance(const std::string& instance) { return (instance != "nope"); } + virtual bool HandleTrailingStep() + { + if (HasTrailingStep()) + { + if (trailingStepDone_) + { + throw OrthancException(ErrorCode_InternalError); + } + else + { + trailingStepDone_ = true; + return true; + } + } + else + { + throw OrthancException(ErrorCode_InternalError); + } + } + public: - DummyInstancesJob() + DummyInstancesJob(bool hasTrailingStep) : + SetOfInstancesJob(hasTrailingStep), + trailingStepDone_(false) { } DummyInstancesJob(const Json::Value& value) : SetOfInstancesJob(value) { + if (HasTrailingStep()) + { + trailingStepDone_ = (GetPosition() == GetStepsCount()); + } + else + { + trailingStepDone_ = false; + } + } + + bool IsTrailingStepDone() const + { + return trailingStepDone_; } virtual void Stop(JobStopReason reason) @@ -790,7 +828,38 @@ Json::Value b = 43; if (unserialized->Serialize(b)) { - return CheckSameJson(a, b); + return (CheckSameJson(a, b)); + } + else + { + return false; + } + } +} + + +static bool CheckIdempotentSetOfInstances(IJobUnserializer& unserializer, + SetOfInstancesJob& job) +{ + Json::Value a = 42; + + if (!job.Serialize(a)) + { + return false; + } + else + { + std::auto_ptr unserialized + (dynamic_cast(unserializer.UnserializeJob(a))); + + Json::Value b = 43; + if (unserialized->Serialize(b)) + { + return (CheckSameJson(a, b) && + job.HasTrailingStep() == unserialized->HasTrailingStep() && + job.GetPosition() == unserialized->GetPosition() && + job.GetInstancesCount() == unserialized->GetInstancesCount() && + job.GetStepsCount() == unserialized->GetStepsCount()); } else { @@ -956,20 +1025,22 @@ // This tests SetOfInstancesJob { - DummyInstancesJob job; + DummyInstancesJob job(false); job.SetDescription("description"); job.AddInstance("hello"); job.AddInstance("nope"); job.AddInstance("world"); job.SetPermissive(true); ASSERT_THROW(job.Step(), OrthancException); // Not started yet + ASSERT_FALSE(job.HasTrailingStep()); + ASSERT_FALSE(job.IsTrailingStepDone()); job.Start(); job.Step(); job.Step(); { DummyUnserializer unserializer; - ASSERT_TRUE(CheckIdempotentSerialization(unserializer, job)); + ASSERT_TRUE(CheckIdempotentSetOfInstances(unserializer, job)); } ASSERT_TRUE(job.Serialize(s)); @@ -1418,7 +1489,7 @@ job.SetRemoteModality(modality); job.SetMoveOriginator("MOVESCU", 42); - ASSERT_TRUE(CheckIdempotentSerialization(unserializer, job)); + ASSERT_TRUE(CheckIdempotentSetOfInstances(unserializer, job)); ASSERT_TRUE(job.Serialize(s)); } @@ -1448,7 +1519,7 @@ OrthancPeerStoreJob job(GetContext()); job.SetPeer(peer); - ASSERT_TRUE(CheckIdempotentSerialization(unserializer, job)); + ASSERT_TRUE(CheckIdempotentSetOfInstances(unserializer, job)); ASSERT_TRUE(job.Serialize(s)); } @@ -1473,7 +1544,7 @@ job.SetModification(modification.release(), true); job.SetOrigin(DicomInstanceOrigin::FromLua()); - ASSERT_TRUE(CheckIdempotentSerialization(unserializer, job)); + ASSERT_TRUE(CheckIdempotentSetOfInstances(unserializer, job)); ASSERT_TRUE(job.Serialize(s)); } @@ -1510,3 +1581,144 @@ ASSERT_TRUE(CheckSameJson(s, t)); } } + + +TEST(JobsSerialization, TrailingStep) +{ + { + Json::Value s; + + DummyInstancesJob job(false); + ASSERT_EQ(0, job.GetStepsCount()); + ASSERT_EQ(0, job.GetInstancesCount()); + + job.Start(); + ASSERT_EQ(0, job.GetPosition()); + ASSERT_FALSE(job.HasTrailingStep()); + ASSERT_FALSE(job.IsTrailingStepDone()); + + { + DummyUnserializer unserializer; + ASSERT_TRUE(CheckIdempotentSetOfInstances(unserializer, job)); + } + + ASSERT_EQ(JobStepCode_Success, job.Step().GetCode()); + ASSERT_EQ(1, job.GetPosition()); + ASSERT_FALSE(job.IsTrailingStepDone()); + + { + DummyUnserializer unserializer; + ASSERT_TRUE(CheckIdempotentSetOfInstances(unserializer, job)); + } + + ASSERT_THROW(job.Step(), OrthancException); + } + + { + Json::Value s; + + DummyInstancesJob job(false); + job.AddInstance("hello"); + job.AddInstance("world"); + ASSERT_EQ(2, job.GetStepsCount()); + ASSERT_EQ(2, job.GetInstancesCount()); + + job.Start(); + ASSERT_EQ(0, job.GetPosition()); + ASSERT_FALSE(job.HasTrailingStep()); + ASSERT_FALSE(job.IsTrailingStepDone()); + + { + DummyUnserializer unserializer; + ASSERT_TRUE(CheckIdempotentSetOfInstances(unserializer, job)); + } + + ASSERT_EQ(JobStepCode_Continue, job.Step().GetCode()); + ASSERT_EQ(1, job.GetPosition()); + ASSERT_FALSE(job.IsTrailingStepDone()); + + { + DummyUnserializer unserializer; + ASSERT_TRUE(CheckIdempotentSetOfInstances(unserializer, job)); + } + + ASSERT_EQ(JobStepCode_Success, job.Step().GetCode()); + ASSERT_EQ(2, job.GetPosition()); + ASSERT_FALSE(job.IsTrailingStepDone()); + + { + DummyUnserializer unserializer; + ASSERT_TRUE(CheckIdempotentSetOfInstances(unserializer, job)); + } + + ASSERT_THROW(job.Step(), OrthancException); + } + + { + Json::Value s; + + DummyInstancesJob job(true); + ASSERT_EQ(1, job.GetStepsCount()); + ASSERT_EQ(0, job.GetInstancesCount()); + + job.Start(); + ASSERT_EQ(0, job.GetPosition()); + ASSERT_TRUE(job.HasTrailingStep()); + ASSERT_FALSE(job.IsTrailingStepDone()); + + { + DummyUnserializer unserializer; + ASSERT_TRUE(CheckIdempotentSetOfInstances(unserializer, job)); + } + + ASSERT_EQ(JobStepCode_Success, job.Step().GetCode()); + ASSERT_EQ(1, job.GetPosition()); + ASSERT_TRUE(job.IsTrailingStepDone()); + + { + DummyUnserializer unserializer; + ASSERT_TRUE(CheckIdempotentSetOfInstances(unserializer, job)); + } + + ASSERT_THROW(job.Step(), OrthancException); + } + + { + Json::Value s; + + DummyInstancesJob job(true); + job.AddInstance("hello"); + ASSERT_EQ(2, job.GetStepsCount()); + ASSERT_EQ(1, job.GetInstancesCount()); + + job.Start(); + ASSERT_EQ(0, job.GetPosition()); + ASSERT_TRUE(job.HasTrailingStep()); + ASSERT_FALSE(job.IsTrailingStepDone()); + + { + DummyUnserializer unserializer; + ASSERT_TRUE(CheckIdempotentSetOfInstances(unserializer, job)); + } + + ASSERT_EQ(JobStepCode_Continue, job.Step().GetCode()); + ASSERT_EQ(1, job.GetPosition()); + ASSERT_FALSE(job.IsTrailingStepDone()); + + { + DummyUnserializer unserializer; + ASSERT_TRUE(CheckIdempotentSetOfInstances(unserializer, job)); + } + + ASSERT_EQ(JobStepCode_Success, job.Step().GetCode()); + ASSERT_EQ(2, job.GetPosition()); + ASSERT_TRUE(job.IsTrailingStepDone()); + + { + DummyUnserializer unserializer; + ASSERT_TRUE(CheckIdempotentSetOfInstances(unserializer, job)); + } + + ASSERT_THROW(job.Step(), OrthancException); + } +}