Mercurial > hg > orthanc

changeset 3240:e44e0127e553

Find changesets by keywords (author, files, the commit message), revision number or hash, or revset expression.
Fix issue #134 (/patient/modify gives 500, should really be 400)
author Sebastien Jodogne <s.jodogne@gmail.com>
date Fri, 15 Feb 2019 17:26:45 +0100
parents 407e1a188105
children 32596919d729
files Core/JobsEngine/JobStatus.cpp Core/JobsEngine/JobStatus.h Core/JobsEngine/JobStepResult.cpp Core/JobsEngine/JobStepResult.h Core/JobsEngine/JobsEngine.cpp Core/JobsEngine/JobsRegistry.cpp Core/JobsEngine/JobsRegistry.h Core/JobsEngine/SetOfCommandsJob.cpp NEWS OrthancServer/OrthancRestApi/OrthancRestApi.cpp OrthancServer/OrthancRestApi/OrthancRestArchive.cpp OrthancServer/ServerJobs/ArchiveJob.cpp Plugins/Engine/PluginsJob.cpp UnitTestsSources/MultiThreadingTests.cpp
diffstat 14 files changed, 100 insertions(+), 45 deletions(-) [+]
line wrap: on
line diff
--- 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)
     {
--- 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_;
+    }
   };
 }
--- 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);
+    }
+  }
 }
--- 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;
   };
 }
--- 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:
--- 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();
--- 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);
     };
   };
 }
--- 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);
       }
     }
 
--- 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)
--- 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
     {
--- 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
     {
--- 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)
--- 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();
--- 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();