# HG changeset patch # User Sebastien Jodogne # Date 1550248005 -3600 # Node ID e44e0127e5533f9feecce436444e92eb48b89200 # Parent 407e1a18810556c7cd55d055a06c739286fa5386 Fix issue #134 (/patient/modify gives 500, should really be 400) diff -r 407e1a188105 -r e44e0127e553 Core/JobsEngine/JobStatus.cpp --- a/Core/JobsEngine/JobStatus.cpp Fri Feb 15 16:38:12 2019 +0100 +++ b/Core/JobsEngine/JobStatus.cpp Fri Feb 15 17:26:45 2019 +0100 @@ -49,10 +49,12 @@ JobStatus::JobStatus(ErrorCode code, + const std::string& details, IJob& job) : errorCode_(code), progress_(job.GetProgress()), - publicContent_(Json::objectValue) + publicContent_(Json::objectValue), + details_(details) { if (progress_ < 0) { diff -r 407e1a188105 -r e44e0127e553 Core/JobsEngine/JobStatus.h --- a/Core/JobsEngine/JobStatus.h Fri Feb 15 16:38:12 2019 +0100 +++ b/Core/JobsEngine/JobStatus.h Fri Feb 15 17:26:45 2019 +0100 @@ -46,11 +46,13 @@ Json::Value publicContent_; Json::Value serialized_; bool hasSerialized_; + std::string details_; public: JobStatus(); JobStatus(ErrorCode code, + const std::string& details, IJob& job); ErrorCode GetErrorCode() const @@ -84,5 +86,10 @@ { return hasSerialized_; } + + const std::string& GetDetails() const + { + return details_; + } }; } diff -r 407e1a188105 -r e44e0127e553 Core/JobsEngine/JobStepResult.cpp --- a/Core/JobsEngine/JobStepResult.cpp Fri Feb 15 16:38:12 2019 +0100 +++ b/Core/JobsEngine/JobStepResult.cpp Fri Feb 15 17:26:45 2019 +0100 @@ -46,14 +46,28 @@ } - JobStepResult JobStepResult::Failure(const ErrorCode& error) + JobStepResult JobStepResult::Failure(const ErrorCode& error, + const char* details) { JobStepResult result(JobStepCode_Failure); result.error_ = error; + + if (details != NULL) + { + result.failureDetails_ = details; + } + return result; } + JobStepResult JobStepResult::Failure(const OrthancException& exception) + { + return Failure(exception.GetErrorCode(), + exception.HasDetails() ? exception.GetDetails() : NULL); + } + + unsigned int JobStepResult::GetRetryTimeout() const { if (code_ == JobStepCode_Retry) @@ -78,4 +92,17 @@ throw OrthancException(ErrorCode_BadSequenceOfCalls); } } + + + const std::string& JobStepResult::GetFailureDetails() const + { + if (code_ == JobStepCode_Failure) + { + return failureDetails_; + } + else + { + throw OrthancException(ErrorCode_BadSequenceOfCalls); + } + } } diff -r 407e1a188105 -r e44e0127e553 Core/JobsEngine/JobStepResult.h --- a/Core/JobsEngine/JobStepResult.h Fri Feb 15 16:38:12 2019 +0100 +++ b/Core/JobsEngine/JobStepResult.h Fri Feb 15 17:26:45 2019 +0100 @@ -37,12 +37,15 @@ namespace Orthanc { + class OrthancException; + class JobStepResult { private: JobStepCode code_; unsigned int timeout_; ErrorCode error_; + std::string failureDetails_; explicit JobStepResult(JobStepCode code) : code_(code), @@ -71,7 +74,10 @@ static JobStepResult Retry(unsigned int timeout); - static JobStepResult Failure(const ErrorCode& error); + static JobStepResult Failure(const ErrorCode& error, + const char* details); + + static JobStepResult Failure(const OrthancException& exception); JobStepCode GetCode() const { @@ -81,5 +87,7 @@ unsigned int GetRetryTimeout() const; ErrorCode GetFailureCode() const; + + const std::string& GetFailureDetails() const; }; } diff -r 407e1a188105 -r e44e0127e553 Core/JobsEngine/JobsEngine.cpp --- a/Core/JobsEngine/JobsEngine.cpp Fri Feb 15 16:38:12 2019 +0100 +++ b/Core/JobsEngine/JobsEngine.cpp Fri Feb 15 17:26:45 2019 +0100 @@ -75,39 +75,39 @@ } catch (OrthancException& e) { - result = JobStepResult::Failure(e.GetErrorCode()); + result = JobStepResult::Failure(e); } catch (boost::bad_lexical_cast&) { - result = JobStepResult::Failure(ErrorCode_BadFileFormat); + result = JobStepResult::Failure(ErrorCode_BadFileFormat, NULL); } catch (...) { - result = JobStepResult::Failure(ErrorCode_InternalError); + result = JobStepResult::Failure(ErrorCode_InternalError, NULL); } switch (result.GetCode()) { case JobStepCode_Success: running.GetJob().Stop(JobStopReason_Success); - running.UpdateStatus(ErrorCode_Success); + running.UpdateStatus(ErrorCode_Success, ""); running.MarkSuccess(); return false; case JobStepCode_Failure: running.GetJob().Stop(JobStopReason_Failure); - running.UpdateStatus(result.GetFailureCode()); + running.UpdateStatus(result.GetFailureCode(), result.GetFailureDetails()); running.MarkFailure(); return false; case JobStepCode_Retry: running.GetJob().Stop(JobStopReason_Retry); - running.UpdateStatus(ErrorCode_Success); + running.UpdateStatus(ErrorCode_Success, ""); running.MarkRetry(result.GetRetryTimeout()); return false; case JobStepCode_Continue: - running.UpdateStatus(ErrorCode_Success); + running.UpdateStatus(ErrorCode_Success, ""); return true; default: diff -r 407e1a188105 -r e44e0127e553 Core/JobsEngine/JobsRegistry.cpp --- a/Core/JobsEngine/JobsRegistry.cpp Fri Feb 15 16:38:12 2019 +0100 +++ b/Core/JobsEngine/JobsRegistry.cpp Fri Feb 15 17:26:45 2019 +0100 @@ -110,7 +110,7 @@ job->GetJobType(jobType_); job->Start(); - lastStatus_ = JobStatus(ErrorCode_Success, *job_); + lastStatus_ = JobStatus(ErrorCode_Success, "", *job_); } const std::string& GetId() const @@ -318,7 +318,7 @@ job_->GetJobType(jobType_); job_->Start(); - lastStatus_ = JobStatus(ErrorCode_Success, *job_); + lastStatus_ = JobStatus(ErrorCode_Success, "", *job_); } }; @@ -739,7 +739,7 @@ } - bool JobsRegistry::SubmitAndWait(Json::Value& successContent, + void JobsRegistry::SubmitAndWait(Json::Value& successContent, IJob* job, // Takes ownership int priority) { @@ -764,7 +764,25 @@ else if (state == JobState_Failure) { // Failure - break; + JobsIndex::const_iterator it = jobsIndex_.find(id); + if (it != jobsIndex_.end()) // Should always be true, already tested in GetStateInternal() + { + ErrorCode code = it->second->GetLastStatus().GetErrorCode(); + const std::string& details = it->second->GetLastStatus().GetDetails(); + + if (details.empty()) + { + throw OrthancException(code); + } + else + { + throw OrthancException(code, details); + } + } + else + { + throw OrthancException(ErrorCode_InternalError); + } } else if (state == JobState_Success) { @@ -781,7 +799,7 @@ successContent = status.GetPublicContent(); } - break; + return; } else { @@ -790,8 +808,6 @@ } } } - - return (state == JobState_Success); } @@ -1320,7 +1336,8 @@ } - void JobsRegistry::RunningJob::UpdateStatus(ErrorCode code) + void JobsRegistry::RunningJob::UpdateStatus(ErrorCode code, + const std::string& details) { if (!IsValid()) { @@ -1328,7 +1345,7 @@ } else { - JobStatus status(code, *job_); + JobStatus status(code, details, *job_); boost::mutex::scoped_lock lock(registry_.mutex_); registry_.CheckInvariants(); diff -r 407e1a188105 -r e44e0127e553 Core/JobsEngine/JobsRegistry.h --- a/Core/JobsEngine/JobsRegistry.h Fri Feb 15 16:38:12 2019 +0100 +++ b/Core/JobsEngine/JobsRegistry.h Fri Feb 15 17:26:45 2019 +0100 @@ -175,7 +175,7 @@ void Submit(IJob* job, // Takes ownership int priority); - bool SubmitAndWait(Json::Value& successContent, + void SubmitAndWait(Json::Value& successContent, IJob* job, // Takes ownership int priority); @@ -248,7 +248,8 @@ void MarkRetry(unsigned int timeout); - void UpdateStatus(ErrorCode code); + void UpdateStatus(ErrorCode code, + const std::string& details); }; }; } diff -r 407e1a188105 -r e44e0127e553 Core/JobsEngine/SetOfCommandsJob.cpp --- a/Core/JobsEngine/SetOfCommandsJob.cpp Fri Feb 15 16:38:12 2019 +0100 +++ b/Core/JobsEngine/SetOfCommandsJob.cpp Fri Feb 15 17:26:45 2019 +0100 @@ -174,7 +174,7 @@ // Error if (!permissive_) { - return JobStepResult::Failure(ErrorCode_InternalError); + return JobStepResult::Failure(ErrorCode_InternalError, NULL); } } } @@ -186,7 +186,7 @@ } else { - return JobStepResult::Failure(e.GetErrorCode()); + return JobStepResult::Failure(e); } } diff -r 407e1a188105 -r e44e0127e553 NEWS --- a/NEWS Fri Feb 15 16:38:12 2019 +0100 +++ b/NEWS Fri Feb 15 17:26:45 2019 +0100 @@ -7,6 +7,7 @@ - Korean (ISO 2022 IR 149) - Simplified Chinese (ISO 2022 IR 58) * Basic support for character sets with code extensions (ISO 2022 escape sequences) +* Fix issue #134 (/patient/modify gives 500, should really be 400) Version 1.5.4 (2019-02-08) diff -r 407e1a188105 -r e44e0127e553 OrthancServer/OrthancRestApi/OrthancRestApi.cpp --- a/OrthancServer/OrthancRestApi/OrthancRestApi.cpp Fri Feb 15 16:38:12 2019 +0100 +++ b/OrthancServer/OrthancRestApi/OrthancRestApi.cpp Fri Feb 15 17:26:45 2019 +0100 @@ -250,17 +250,11 @@ if (synchronous) { Json::Value successContent; - if (context.GetJobsEngine().GetRegistry().SubmitAndWait - (successContent, raii.release(), priority)) - { - // Success in synchronous execution - output.AnswerJson(successContent); - } - else - { - // Error during synchronous execution - output.SignalError(HttpStatus_500_InternalServerError); - } + context.GetJobsEngine().GetRegistry().SubmitAndWait + (successContent, raii.release(), priority); + + // Success in synchronous execution + output.AnswerJson(successContent); } else { diff -r 407e1a188105 -r e44e0127e553 OrthancServer/OrthancRestApi/OrthancRestArchive.cpp --- a/OrthancServer/OrthancRestApi/OrthancRestArchive.cpp Fri Feb 15 16:38:12 2019 +0100 +++ b/OrthancServer/OrthancRestApi/OrthancRestArchive.cpp Fri Feb 15 17:26:45 2019 +0100 @@ -147,8 +147,9 @@ job->SetSynchronousTarget(tmp); Json::Value publicContent; - if (context.GetJobsEngine().GetRegistry().SubmitAndWait - (publicContent, job.release(), priority)) + context.GetJobsEngine().GetRegistry().SubmitAndWait + (publicContent, job.release(), priority); + { // The archive is now created: Prepare the sending of the ZIP file FilesystemHttpSender sender(tmp->GetPath(), MimeType_Zip); @@ -157,10 +158,6 @@ // Send the ZIP output.AnswerStream(sender); } - else - { - output.SignalError(HttpStatus_500_InternalServerError); - } } else { diff -r 407e1a188105 -r e44e0127e553 OrthancServer/ServerJobs/ArchiveJob.cpp --- a/OrthancServer/ServerJobs/ArchiveJob.cpp Fri Feb 15 16:38:12 2019 +0100 +++ b/OrthancServer/ServerJobs/ArchiveJob.cpp Fri Feb 15 17:26:45 2019 +0100 @@ -943,7 +943,8 @@ synchronousTarget_.unique()) { LOG(WARNING) << "A client has disconnected while creating an archive"; - return JobStepResult::Failure(ErrorCode_NetworkProtocol); + return JobStepResult::Failure(ErrorCode_NetworkProtocol, + "A client has disconnected while creating an archive"); } if (writer_->GetStepsCount() == 0) diff -r 407e1a188105 -r e44e0127e553 Plugins/Engine/PluginsJob.cpp --- a/Plugins/Engine/PluginsJob.cpp Fri Feb 15 16:38:12 2019 +0100 +++ b/Plugins/Engine/PluginsJob.cpp Fri Feb 15 17:26:45 2019 +0100 @@ -88,7 +88,7 @@ return JobStepResult::Success(); case OrthancPluginJobStepStatus_Failure: - return JobStepResult::Failure(ErrorCode_Plugin); + return JobStepResult::Failure(ErrorCode_Plugin, NULL); case OrthancPluginJobStepStatus_Continue: return JobStepResult::Continue(); diff -r 407e1a188105 -r e44e0127e553 UnitTestsSources/MultiThreadingTests.cpp --- a/UnitTestsSources/MultiThreadingTests.cpp Fri Feb 15 16:38:12 2019 +0100 +++ b/UnitTestsSources/MultiThreadingTests.cpp Fri Feb 15 17:26:45 2019 +0100 @@ -105,7 +105,7 @@ { if (fails_) { - return JobStepResult::Failure(ErrorCode_ParameterOutOfRange); + return JobStepResult::Failure(ErrorCode_ParameterOutOfRange, NULL); } else if (count_ == steps_ - 1) { @@ -724,12 +724,12 @@ engine.Start(); Json::Value content = Json::nullValue; - ASSERT_TRUE(engine.GetRegistry().SubmitAndWait(content, new DummyJob(), rand() % 10)); + engine.GetRegistry().SubmitAndWait(content, new DummyJob(), rand() % 10); ASSERT_EQ(Json::objectValue, content.type()); ASSERT_EQ("world", content["hello"].asString()); content = Json::nullValue; - ASSERT_FALSE(engine.GetRegistry().SubmitAndWait(content, new DummyJob(true), rand() % 10)); + ASSERT_THROW(engine.GetRegistry().SubmitAndWait(content, new DummyJob(true), rand() % 10), OrthancException); ASSERT_EQ(Json::nullValue, content.type()); engine.Stop();