SONAR-2706 improve error handling on command execution

diff --git a/sonar-core/src/main/java/org/sonar/core/review/workflow/WorkflowEngine.java b/sonar-core/src/main/java/org/sonar/core/review/workflow/WorkflowEngine.java
index 0766be4ed08d9dac8f680e6d1b039e498711c980..4c7c17fec176487a6e104a413c972be911a41777 100644 (file)
--- a/sonar-core/src/main/java/org/sonar/core/review/workflow/WorkflowEngine.java
+++ b/sonar-core/src/main/java/org/sonar/core/review/workflow/WorkflowEngine.java
@@ -58,9 +58,9 @@ public class WorkflowEngine implements ServerComponent {
 
     for (Map.Entry<String, Screen> entry : workflow.getScreensByCommand().entrySet()) {
       String commandKey = entry.getKey();
-      if (!verifyConditions || verifyConditions(null, context, workflow.getContextConditions(commandKey))) {
+      if (!verifyConditions || verifyConditionsQuietly(null, context, workflow.getContextConditions(commandKey))) {
         for (Review review : reviews) {
-          if (!verifyConditions || verifyConditions(review, context, workflow.getReviewConditions(commandKey))) {
+          if (!verifyConditions || verifyConditionsQuietly(review, context, workflow.getReviewConditions(commandKey))) {
             result.put(review.getViolationId(), entry.getValue());
           }
         }
@@ -74,7 +74,7 @@ public class WorkflowEngine implements ServerComponent {
     completeProjectSettings(context);
     for (Map.Entry<String, Screen> entry : workflow.getScreensByCommand().entrySet()) {
       String commandKey = entry.getKey();
-      if (!verifyConditions || verifyConditions(review, context, workflow.getConditions(commandKey))) {
+      if (!verifyConditions || verifyConditionsQuietly(review, context, workflow.getConditions(commandKey))) {
         result.add(entry.getValue());
 
       }
@@ -95,7 +95,7 @@ public class WorkflowEngine implements ServerComponent {
 
     completeProjectSettings(context);
 
-    Preconditions.checkState(verifyConditions(review, context, workflow.getConditions(commandKey)), "Conditions are not respected");
+    verifyConditions(review, context, workflow.getConditions(commandKey));
 
     Map<String, String> immutableParameters = ImmutableMap.copyOf(parameters);
 
@@ -111,13 +111,21 @@ public class WorkflowEngine implements ServerComponent {
     // TODO notify listeners
   }
 
-  private boolean verifyConditions(@Nullable Review review, WorkflowContext context, List<Condition> conditions) {
+  private boolean verifyConditionsQuietly(@Nullable Review review, WorkflowContext context, List<Condition> conditions) {
+      for (Condition condition : conditions) {
+        if (!condition.doVerify(review, context)) {
+          return false;
+        }
+      }
+      return true;
+    }
+
+  private void verifyConditions(@Nullable Review review, WorkflowContext context, List<Condition> conditions) {
     for (Condition condition : conditions) {
       if (!condition.doVerify(review, context)) {
-        return false;
+        throw new IllegalStateException("Condition is not respected: " + condition.toString());
       }
     }
-    return true;
   }
 
   private void completeProjectSettings(DefaultWorkflowContext context) {

diff --git a/sonar-core/src/main/java/org/sonar/core/review/workflow/condition/HasProjectPropertyCondition.java b/sonar-core/src/main/java/org/sonar/core/review/workflow/condition/HasProjectPropertyCondition.java
index 40a89b1a0abc6a1a7ddf1dbbd698ac1b193af8b0..c30cbd3ff697a9c9ab30b454e4e7d16142b62c95 100644 (file)
--- a/sonar-core/src/main/java/org/sonar/core/review/workflow/condition/HasProjectPropertyCondition.java
+++ b/sonar-core/src/main/java/org/sonar/core/review/workflow/condition/HasProjectPropertyCondition.java
@@ -41,4 +41,8 @@ public final class HasProjectPropertyCondition extends ProjectPropertyCondition
     Settings settings = context.getProjectSettings();
     return settings.hasKey(getPropertyKey()) || settings.getDefaultValue(getPropertyKey()) != null;
   }
+
+  public String toString() {
+    return "Property " + getPropertyKey() + " must be set";
+  }
 }

diff --git a/sonar-core/src/test/java/org/sonar/core/review/workflow/WorkflowEngineTest.java b/sonar-core/src/test/java/org/sonar/core/review/workflow/WorkflowEngineTest.java
index e290e8bd6aa3f0084c8c060a0198851ab5e55b7d..6e8c1950f255854c0181090e0a1f6394306c0cf5 100644 (file)
--- a/sonar-core/src/test/java/org/sonar/core/review/workflow/WorkflowEngineTest.java
+++ b/sonar-core/src/test/java/org/sonar/core/review/workflow/WorkflowEngineTest.java
@@ -150,7 +150,7 @@ public class WorkflowEngineTest {
   @Test
   public void execute_fail_if_conditions_dont_pass() {
     thrown.expect(IllegalStateException.class);
-    thrown.expectMessage("Conditions are not respected");
+    thrown.expectMessage("Condition is not respected: Property foo must be set");
 
     Workflow workflow = new Workflow();
     workflow.addCommand("resolve");

diff --git a/sonar-server/src/main/webapp/WEB-INF/app/controllers/api/api_controller.rb b/sonar-server/src/main/webapp/WEB-INF/app/controllers/api/api_controller.rb
index bc7e9ec7cd505c00e38f8cb65782f952904fcbcc..3f3f1a1f7469574ba1da8cc98a266f6abf1c6c86 100644 (file)
--- a/sonar-server/src/main/webapp/WEB-INF/app/controllers/api/api_controller.rb
+++ b/sonar-server/src/main/webapp/WEB-INF/app/controllers/api/api_controller.rb
@@ -79,7 +79,7 @@ class Api::ApiController < ApplicationController
   #
 
   def render_error(exception, status=500)
-    message = exception.respond_to?('message') ? Api::Utils.exception_message(exception) : exception.to_s
+    message = exception.respond_to?('message') ? Api::Utils.exception_message(exception, :backtrace => true) : exception.to_s
     java_facade.logError("Fail to render: #{request.url}\n#{message}") if status==500
     respond_to do |format|
       format.json { render :json => error_to_json(status, message), :status => status }

diff --git a/sonar-server/src/main/webapp/WEB-INF/app/controllers/application_controller.rb b/sonar-server/src/main/webapp/WEB-INF/app/controllers/application_controller.rb
index 3b382a1b3245cb6aa7cdeba724111efba40634ab..8636de40e54ffa15de300044c73c5b42457feeab 100644 (file)
--- a/sonar-server/src/main/webapp/WEB-INF/app/controllers/application_controller.rb
+++ b/sonar-server/src/main/webapp/WEB-INF/app/controllers/application_controller.rb
@@ -145,7 +145,7 @@ class ApplicationController < ActionController::Base
   def render_error(error)
     # Ruby on Rails has a single logger "rails", so it's not possible to distinguish profiling logs
     # from error logs. For this reason a standard SLF4J logger is used instead of logger.error().
-    java_facade.logError("Fail to render: #{request.url}\n#{Api::Utils.exception_message(error)}")
+    java_facade.logError("Fail to render: #{request.url}\n#{Api::Utils.exception_message(error, :backtrace => true)}")
 
     if request.xhr?
       message = error.respond_to?('message') ? error.message : error.to_s

diff --git a/sonar-server/src/main/webapp/WEB-INF/app/controllers/project_reviews_controller.rb b/sonar-server/src/main/webapp/WEB-INF/app/controllers/project_reviews_controller.rb
index b53541094a6b008e51251a4aed54b864dbf174c6..68d1818e4e59fa239f7862ad3392b3d096849fc3 100644 (file)
--- a/sonar-server/src/main/webapp/WEB-INF/app/controllers/project_reviews_controller.rb
+++ b/sonar-server/src/main/webapp/WEB-INF/app/controllers/project_reviews_controller.rb
@@ -266,10 +266,15 @@ class ProjectReviewsController < ApplicationController
     access_denied unless has_rights_to_modify?(@review.resource)
 
     bad_request('Missing command') if params[:command].blank?
-    RuleFailure.execute_command(params[:command], @review.violation, @review.resource.project, current_user, params)
 
+    error_message = nil
+    begin
+      RuleFailure.execute_command(params[:command], @review.violation, @review.resource.project, current_user, params)
+    rescue Exception => e
+      error_message=Api::Utils.exception_message(e, :backtrace => false)
+    end
     @review.reload
-    render :partial => "project_reviews/view"
+    render :partial => "project_reviews/view", :locals => {:review => @review, :error_message => error_message}
   end
 
 

diff --git a/sonar-server/src/main/webapp/WEB-INF/app/controllers/reviews_controller.rb b/sonar-server/src/main/webapp/WEB-INF/app/controllers/reviews_controller.rb
index cfc7eccb2a682013f494b6a55714969a2a4a7c98..d30e078e7e9f208f89f633d6ab57438172ea519c 100644 (file)
--- a/sonar-server/src/main/webapp/WEB-INF/app/controllers/reviews_controller.rb
+++ b/sonar-server/src/main/webapp/WEB-INF/app/controllers/reviews_controller.rb
@@ -68,14 +68,19 @@ class ReviewsController < ApplicationController
       violation.create_review!(:user_id => current_user.id)
     end
 
-    # TODO remove parameters id and command from params
-    RuleFailure.execute_command(params[:command], violation, violation.snapshot.root_snapshot.project, current_user, params)
+    # TODO remove parameters 'id' and 'command' from params
+    error_message = nil
+    begin
+      RuleFailure.execute_command(params[:command], violation, violation.snapshot.root_snapshot.project, current_user, params)
+    rescue Exception => e
+      error_message=Api::Utils.exception_message(e, :backtrace => false)
+    end
 
     # reload data required for display
     violation.reload
     screens = violation.available_java_screens(current_user)
 
-    render :partial => 'resource/violation', :locals => {:violation => violation, :review_screens => screens}
+    render :partial => 'resource/violation', :locals => {:violation => violation, :review_screens => screens, :error_message => error_message}
   end
 
   #
@@ -200,12 +205,7 @@ class ReviewsController < ApplicationController
       if params[:comment_id]
         violation.review.edit_comment(current_user, params[:comment_id].to_i, params[:text])
       else
-        begin
-          violation.review.create_comment({:user => current_user, :text => params[:text]}, params[:review_command_id])
-        rescue Exception => e
-          # the review command may throw an exception
-          flash["review_error_#{violation.id}"] = e.clean_message
-        end
+        violation.review.create_comment({:user => current_user, :text => params[:text]}, params[:review_command_id])
       end
     end
 

diff --git a/sonar-server/src/main/webapp/WEB-INF/app/models/api/utils.rb b/sonar-server/src/main/webapp/WEB-INF/app/models/api/utils.rb
index 7ceec9ad93b8d243046d170327c384498308b945..fd7bc1d432d2e1f8fa3ec4e3d3ec56f5b1932c5a 100644 (file)
--- a/sonar-server/src/main/webapp/WEB-INF/app/models/api/utils.rb
+++ b/sonar-server/src/main/webapp/WEB-INF/app/models/api/utils.rb
@@ -70,10 +70,18 @@ class Api::Utils
     Java::OrgSonarServerUi::JRubyFacade.getInstance().getMessage(I18n.locale, key, default, params.to_java)
   end
 
-  def self.exception_message(exception)
-    result = (exception.respond_to?(:message) ? "#{exception.message}\n" : "#{exception}\n")
-    if exception.respond_to? :backtrace
-      result << "\t" + exception.backtrace.join("\n\t") + "\n"
+  #
+  # Options :
+  # - backtrace: append backtrace if true. Default value is false.
+  #
+  def self.exception_message(exception, options={})
+    cause = exception
+    if exception.is_a?(NativeException) && exception.respond_to?(:cause)
+      cause = exception.cause
+    end
+    result = (cause.respond_to?(:message) ? "#{cause.message}\n" : "#{cause}\n")
+    if options[:backtrace]==true && cause.respond_to?(:backtrace)
+      result << "\t" + cause.backtrace.join("\n\t") + "\n"
     end
     result
   end

diff --git a/sonar-server/src/main/webapp/WEB-INF/app/views/project_reviews/_review.html.erb b/sonar-server/src/main/webapp/WEB-INF/app/views/project_reviews/_review.html.erb
index 2a159fb6ec7c9b680df46dccae95e4d466735659..0dd5a5aa4810e1060f55c69b259034e80ae5ab61 100644 (file)
--- a/sonar-server/src/main/webapp/WEB-INF/app/views/project_reviews/_review.html.erb
+++ b/sonar-server/src/main/webapp/WEB-INF/app/views/project_reviews/_review.html.erb
@@ -3,8 +3,8 @@
     <h2><%= message('reviews.review_number', :params => h(review.id.to_s)) -%> - <%= h(review.title) -%></h2>
   </div>
 
-  <% if flash[:review_error] %>
-    <div id="review_error" class="error"><%= flash[:review_error] %> <a href="#" onclick="$('review_error').hide(); return false;"><%= message('reviews.hide_this_message') -%></a></div>
+  <% if defined?(error_message) && error_message %>
+    <div id="review_error" class="error"><%= h error_message -%> <a href="#" onclick="$('review_error').hide(); return false;"><%= message('reviews.hide_this_message') -%></a></div>
   <% end %>
 
   <%

diff --git a/sonar-server/src/main/webapp/WEB-INF/app/views/resource/_violation.html.erb b/sonar-server/src/main/webapp/WEB-INF/app/views/resource/_violation.html.erb
index 3ae0d57e824aec37cfcdd1039eb4947ca6081f22..514ab668cb7d37dda80eaae52fa715fa50670562 100644 (file)
--- a/sonar-server/src/main/webapp/WEB-INF/app/views/resource/_violation.html.erb
+++ b/sonar-server/src/main/webapp/WEB-INF/app/views/resource/_violation.html.erb
@@ -106,12 +106,12 @@
        end
     %>
 
-    <% if flash["review_error_#{violation.id}"] %>
-      <div id="review_error_<%= violation.id -%>" class="error"><%= flash["review_error_#{violation.id}"] %> <a href="#" onclick="$('review_error_<%= violation.id -%>').hide(); return false;"><%= message('reviews.hide_this_message') -%></a></div>
-    <% end %>
-
     <% if current_user %>
       <div class="vActions" id="vActions<%= violation.id -%>">
+        <% if defined?(error_message) && error_message %>
+          <div id="review_error_<%= violation.id -%>" class="error"><%= h error_message -%> <a href="#" onclick="$('review_error_<%= violation.id -%>').hide(); return false;"><%= message('reviews.hide_this_message') -%></a></div>
+        <% end %>
+
         <%= link_to_function message('reviews.comment'), "sCF(#{violation.id})", :name => 'bComment', :class => 'link-action spacer-right' -%>
 
         <% unless violation.review && violation.review.resolved? %>