From 99e41b12a0200c8495de634f9202aea5edfd1ed4 Mon Sep 17 00:00:00 2001 From: Simon Brandhof Date: Wed, 23 May 2012 11:31:14 +0200 Subject: [PATCH] SONAR-2706 improve error handling on command execution --- .../core/review/workflow/WorkflowEngine.java | 22 +++++++++++++------ .../HasProjectPropertyCondition.java | 4 ++++ .../review/workflow/WorkflowEngineTest.java | 2 +- .../app/controllers/api/api_controller.rb | 2 +- .../app/controllers/application_controller.rb | 2 +- .../controllers/project_reviews_controller.rb | 9 ++++++-- .../app/controllers/reviews_controller.rb | 18 +++++++-------- .../webapp/WEB-INF/app/models/api/utils.rb | 16 ++++++++++---- .../views/project_reviews/_review.html.erb | 4 ++-- .../app/views/resource/_violation.html.erb | 8 +++---- 10 files changed, 56 insertions(+), 31 deletions(-) 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 0766be4ed08..4c7c17fec17 100644 --- 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 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 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 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 conditions) { + private boolean verifyConditionsQuietly(@Nullable Review review, WorkflowContext context, List conditions) { + for (Condition condition : conditions) { + if (!condition.doVerify(review, context)) { + return false; + } + } + return true; + } + + private void verifyConditions(@Nullable Review review, WorkflowContext context, List 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 40a89b1a0ab..c30cbd3ff69 100644 --- 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 e290e8bd6aa..6e8c1950f25 100644 --- 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 bc7e9ec7cd5..3f3f1a1f746 100644 --- 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 3b382a1b324..8636de40e54 100644 --- 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 b53541094a6..68d1818e4e5 100644 --- 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 cfc7eccb2a6..d30e078e7e9 100644 --- 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 7ceec9ad93b..fd7bc1d432d 100644 --- 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 2a159fb6ec7..0dd5a5aa481 100644 --- 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 @@

<%= message('reviews.review_number', :params => h(review.id.to_s)) -%> - <%= h(review.title) -%>

- <% if flash[:review_error] %> - + <% if defined?(error_message) && error_message %> + <% 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 3ae0d57e824..514ab668cb7 100644 --- 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}"] %> -
<%= flash["review_error_#{violation.id}"] %> <%= message('reviews.hide_this_message') -%>
- <% end %> - <% if current_user %>
+ <% if defined?(error_message) && error_message %> +
<%= h error_message -%> <%= message('reviews.hide_this_message') -%>
+ <% end %> + <%= link_to_function message('reviews.comment'), "sCF(#{violation.id})", :name => 'bComment', :class => 'link-action spacer-right' -%> <% unless violation.review && violation.review.resolved? %> -- 2.39.5