SONAR-4383 Improve errors handling

sonar-server/src/main/java/org/sonar/server/exceptions/BadRequestException.java

@@ -29,4 +29,9 @@ public class BadRequestException extends HttpException {
   public BadRequestException(String message) {
     super(BAD_REQUEST, message);
   }
+
+  public BadRequestException(String l10nKey, Object... l10nParams) {
+    super(BAD_REQUEST, l10nKey, l10nParams);
+  }
+
 }

sonar-server/src/main/java/org/sonar/server/exceptions/HttpException.java

@@ -19,9 +19,14 @@
  */
 package org.sonar.server.exceptions;
 
+import javax.annotation.CheckForNull;
+
 public class HttpException extends RuntimeException {
   private final int httpCode;
 
+  private String l10nKey;
+  private Object[] l10nParams;
+
   public HttpException(int httpCode) {
     this.httpCode = httpCode;
   }
@@ -31,7 +36,23 @@ public class HttpException extends RuntimeException {
     this.httpCode = httpCode;
   }
 
+  public HttpException(int httpCode, String l10nKey, Object... l10nParams) {
+    this.httpCode = httpCode;
+    this.l10nKey = l10nKey;
+    this.l10nParams = l10nParams;
+  }
+
   public int httpCode() {
     return httpCode;
   }
+
+  @CheckForNull
+  public String l10nKey() {
+    return l10nKey;
+  }
+
+  @CheckForNull
+  public Object[] l10nParams() {
+    return l10nParams;
+  }
 }

sonar-server/src/main/java/org/sonar/server/issue/InternalRubyIssueService.java

@@ -43,6 +43,7 @@ import org.sonar.core.issue.workflow.Transition;
 import org.sonar.core.resource.ResourceDao;
 import org.sonar.core.resource.ResourceDto;
 import org.sonar.core.resource.ResourceQuery;
+import org.sonar.server.exceptions.BadRequestException;
 import org.sonar.server.user.UserSession;
 import org.sonar.server.util.RubyUtils;
 
@@ -458,93 +459,58 @@ public class InternalRubyIssueService implements ServerComponent {
   /**
    * Create issue filter
    */
-  public Result<DefaultIssueFilter> createIssueFilter(Map<String, String> parameters) {
-    Result<DefaultIssueFilter> result = createIssueFilterResultForNew(parameters);
-    if (result.ok()) {
-      try {
-        result.set(issueFilterService.save(result.get(), UserSession.get()));
-      } catch (Exception e) {
-        result.addError(e.getMessage());
-      }
-    }
-    return result;
+  public DefaultIssueFilter createIssueFilter(Map<String, String> parameters) {
+    DefaultIssueFilter result = createIssueFilterResultForNew(parameters);
+    return issueFilterService.save(result, UserSession.get());
   }
 
   /**
    * Update issue filter
    */
-  public Result<DefaultIssueFilter> updateIssueFilter(Map<String, String> parameters) {
-    Result<DefaultIssueFilter> result = createIssueFilterResultForUpdate(parameters);
-    if (result.ok()) {
-      try {
-        result.set(issueFilterService.update(result.get(), UserSession.get()));
-      } catch (Exception e) {
-        result.addError(e.getMessage());
-      }
-    }
-    return result;
+  public DefaultIssueFilter updateIssueFilter(Map<String, String> parameters) {
+    DefaultIssueFilter result = createIssueFilterResultForUpdate(parameters);
+    return issueFilterService.update(result, UserSession.get());
   }
 
   /**
    * Update issue filter data
    */
-  public Result<DefaultIssueFilter> updateIssueFilterQuery(Long issueFilterId, Map<String, Object> data) {
-    Result<DefaultIssueFilter> result = Result.of();
-    try {
-      result.set(issueFilterService.updateFilterQuery(issueFilterId, data, UserSession.get()));
-    } catch (Exception e) {
-      result.addError(e.getMessage());
-    }
-    return result;
+  public DefaultIssueFilter updateIssueFilterQuery(Long issueFilterId, Map<String, Object> data) {
+    return issueFilterService.updateFilterQuery(issueFilterId, data, UserSession.get());
   }
 
   /**
    * Delete issue filter
    */
-  public Result<DefaultIssueFilter> deleteIssueFilter(Long issueFilterId) {
-    Result<DefaultIssueFilter> result = Result.of();
-    try {
-      issueFilterService.delete(issueFilterId, UserSession.get());
-    } catch (Exception e) {
-      result.addError(e.getMessage());
-    }
-    return result;
+  public void deleteIssueFilter(Long issueFilterId) {
+    issueFilterService.delete(issueFilterId, UserSession.get());
   }
 
   /**
    * Copy issue filter
    */
-  public Result<DefaultIssueFilter> copyIssueFilter(Long issueFilterIdToCopy, Map<String, String> parameters) {
-    Result<DefaultIssueFilter> result = createIssueFilterResultForCopy(parameters);
-    if (result.ok()) {
-      try {
-        result.set(issueFilterService.copy(issueFilterIdToCopy, result.get(), UserSession.get()));
-      } catch (Exception e) {
-        result.addError(e.getMessage());
-      }
-    }
-    return result;
+  public DefaultIssueFilter copyIssueFilter(Long issueFilterIdToCopy, Map<String, String> parameters) {
+    DefaultIssueFilter result = createIssueFilterResultForCopy(parameters);
+    return issueFilterService.copy(issueFilterIdToCopy, result, UserSession.get());
   }
 
   @VisibleForTesting
-  Result<DefaultIssueFilter> createIssueFilterResultForNew(Map<String, String> params) {
+  DefaultIssueFilter createIssueFilterResultForNew(Map<String, String> params) {
     return createIssueFilterResult(params, false, false);
   }
 
   @VisibleForTesting
-  Result<DefaultIssueFilter> createIssueFilterResultForUpdate(Map<String, String> params) {
+  DefaultIssueFilter createIssueFilterResultForUpdate(Map<String, String> params) {
     return createIssueFilterResult(params, true, true);
   }
 
   @VisibleForTesting
-  Result<DefaultIssueFilter> createIssueFilterResultForCopy(Map<String, String> params) {
+  DefaultIssueFilter createIssueFilterResultForCopy(Map<String, String> params) {
     return createIssueFilterResult(params, false, false);
   }
 
   @VisibleForTesting
-  Result<DefaultIssueFilter> createIssueFilterResult(Map<String, String> params, boolean checkId, boolean checkUser) {
-    Result<DefaultIssueFilter> result = Result.of();
-
+  DefaultIssueFilter createIssueFilterResult(Map<String, String> params, boolean checkId, boolean checkUser) {
     String id = params.get("id");
     String name = params.get("name");
     String description = params.get("description");
@@ -554,27 +520,23 @@ public class InternalRubyIssueService implements ServerComponent {
     boolean shared = sharedParam != null ? sharedParam : false;
 
     if (checkId) {
-      checkMandatoryParameter(id, "id", result);
+      checkMandatoryParameter(id, "id");
     }
     if (checkUser) {
-      checkMandatoryParameter(user, "user", result);
+      checkMandatoryParameter(user, "user");
     }
-    checkMandatorySizeParameter(name, "name", 100, result);
-    checkOptionalSizeParameter(description, "description", 4000, result);
+    checkMandatorySizeParameter(name, "name", 100);
+    checkOptionalSizeParameter(description, "description", 4000);
 
-    if (result.ok()) {
-      DefaultIssueFilter defaultIssueFilter = DefaultIssueFilter.create(name)
-        .setDescription(description)
-        .setShared(shared)
-        .setUser(user)
-        .setData(data);
-      if (!Strings.isNullOrEmpty(id)) {
-        defaultIssueFilter.setId(Long.valueOf(id));
-      }
-
-      result.set(defaultIssueFilter);
+    DefaultIssueFilter defaultIssueFilter = DefaultIssueFilter.create(name)
+      .setDescription(description)
+      .setShared(shared)
+      .setUser(user)
+      .setData(data);
+    if (!Strings.isNullOrEmpty(id)) {
+      defaultIssueFilter.setId(Long.valueOf(id));
     }
-    return result;
+    return defaultIssueFilter;
   }
 
   public List<DefaultIssueFilter> findSharedFiltersForCurrentUser() {
@@ -587,11 +549,7 @@ public class InternalRubyIssueService implements ServerComponent {
 
   public Result toggleFavouriteIssueFilter(Long issueFilterId) {
     Result result = Result.of();
-    try {
-      issueFilterService.toggleFavouriteIssueFilter(issueFilterId, UserSession.get());
-    } catch (Exception e) {
-      result.addError(e.getMessage());
-    }
+    issueFilterService.toggleFavouriteIssueFilter(issueFilterId, UserSession.get());
     return result;
   }
 
@@ -600,12 +558,8 @@ public class InternalRubyIssueService implements ServerComponent {
    */
   public Result<IssueBulkChangeResult> bulkChange(Map<String, Object> props, String comment) {
     Result<IssueBulkChangeResult> result = Result.of();
-    try {
-      IssueBulkChangeQuery issueBulkChangeQuery = new IssueBulkChangeQuery(props, comment);
-      result.set(issueBulkChangeService.execute(issueBulkChangeQuery, UserSession.get()));
-    } catch (Exception e) {
-      result.addError(e.getMessage());
-    }
+    IssueBulkChangeQuery issueBulkChangeQuery = new IssueBulkChangeQuery(props, comment);
+    result.set(issueBulkChangeService.execute(issueBulkChangeQuery, UserSession.get()));
     return result;
   }
 
@@ -628,4 +582,23 @@ public class InternalRubyIssueService implements ServerComponent {
     }
   }
 
+  private void checkMandatoryParameter(String value, String paramName) {
+    if (Strings.isNullOrEmpty(value)) {
+      throw new BadRequestException("errors.cant_be_empty", paramName);
+    }
+  }
+
+  private void checkMandatorySizeParameter(String value, String paramName, Integer size) {
+    checkMandatoryParameter(value, paramName);
+    if (!Strings.isNullOrEmpty(value) && value.length() > size) {
+      throw new BadRequestException("errors.is_too_long", paramName, size);
+    }
+  }
+
+  private void checkOptionalSizeParameter(String value, String paramName, Integer size) {
+    if (!Strings.isNullOrEmpty(value) && value.length() > size) {
+      throw new BadRequestException("errors.is_too_long", paramName, size);
+    }
+  }
+
 }

sonar-server/src/main/java/org/sonar/server/issue/IssueFilterService.java

@@ -36,6 +36,10 @@ import org.sonar.core.issue.db.IssueFilterFavouriteDao;
 import org.sonar.core.issue.db.IssueFilterFavouriteDto;
 import org.sonar.core.user.AuthorizationDao;
 import org.sonar.core.user.Permission;
+import org.sonar.server.exceptions.BadRequestException;
+import org.sonar.server.exceptions.ForbiddenException;
+import org.sonar.server.exceptions.NotFoundException;
+import org.sonar.server.exceptions.UnauthorizedException;
 import org.sonar.server.user.UserSession;
 
 import javax.annotation.CheckForNull;
@@ -187,8 +191,7 @@ public class IssueFilterService implements ServerComponent {
   private IssueFilterDto findIssueFilterDto(Long id, String login) {
     IssueFilterDto issueFilterDto = filterDao.selectById(id);
     if (issueFilterDto == null) {
-      // TODO throw 404
-      throw new IllegalArgumentException("Filter not found: " + id);
+      throw new NotFoundException("Filter not found: " + id);
     }
     verifyCurrentUserCanReadFilter(issueFilterDto.toIssueFilter(), login);
     return issueFilterDto;
@@ -197,29 +200,26 @@ public class IssueFilterService implements ServerComponent {
   String getLoggedLogin(UserSession userSession) {
     String user = userSession.login();
     if (!userSession.isLoggedIn() && user != null) {
-      throw new IllegalStateException("User is not logged in");
+      throw new UnauthorizedException("User is not logged in");
     }
     return user;
   }
 
   void verifyCurrentUserCanReadFilter(DefaultIssueFilter issueFilter, String login) {
     if (!issueFilter.user().equals(login) && !issueFilter.shared()) {
-      // TODO throw unauthorized
-      throw new IllegalStateException("User is not authorized to read this filter");
+      throw new ForbiddenException("User is not authorized to read this filter");
     }
   }
 
   private void verifyCurrentUserCanModifyFilter(DefaultIssueFilter issueFilter, String user) {
     if (!issueFilter.user().equals(user) && !isAdmin(user)) {
-      // TODO throw unauthorized
-      throw new IllegalStateException("User is not authorized to modify this filter");
+      throw new ForbiddenException("User is not authorized to modify this filter");
     }
   }
 
   private void verifyCurrentUserCanChangeFilterOwnership(String user) {
     if (!isAdmin(user)) {
-      // TODO throw unauthorized
-      throw new IllegalStateException("User is not authorized to change the owner of this filter");
+      throw new ForbiddenException("User is not authorized to change the owner of this filter");
     }
   }
 
@@ -227,13 +227,13 @@ public class IssueFilterService implements ServerComponent {
     List<IssueFilterDto> userFilters = selectUserIssueFilters(issueFilter.user());
     IssueFilterDto userFilterSameName = findFilterWithSameName(userFilters, issueFilter.name());
     if (userFilterSameName != null && !userFilterSameName.getId().equals(issueFilter.id())) {
-      throw new IllegalArgumentException("Name already exists");
+      throw new BadRequestException("Name already exists");
     }
     if (issueFilter.shared()) {
       List<IssueFilterDto> sharedFilters = selectSharedFilters();
       IssueFilterDto sharedFilterWithSameName = findFilterWithSameName(sharedFilters, issueFilter.name());
       if (sharedFilterWithSameName != null && !sharedFilterWithSameName.getId().equals(issueFilter.id())) {
-        throw new IllegalArgumentException("Other users already share filters with the same name");
+        throw new BadRequestException("Other users already share filters with the same name");
       }
     }
   }

sonar-server/src/main/webapp/WEB-INF/app/controllers/api/api_controller.rb

@@ -78,7 +78,12 @@ class Api::ApiController < ApplicationController
   #
   #
 
+  def render_native_access_denied
+    render_access_denied
+  end
+
   def render_error(exception, status=500)
+    # TODO manage exception with l10n parameters
     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
     render_response(status, message)

sonar-server/src/main/webapp/WEB-INF/app/controllers/application_controller.rb

@@ -170,12 +170,21 @@ class ApplicationController < ActionController::Base
   end
 
   def render_server_exception(exception)
-    render :text => exception.getMessage, :status => exception.httpCode
+    message = (exception.getMessage ? exception.getMessage : Api::Utils.message(exception.l10nKey, :params => exception.l10nParams))
+    render :text => message, :status => exception.httpCode
+  end
+
+  def render_native_access_denied
+    access_denied
   end
 
   def render_native_exception(error)
     if error.cause.java_kind_of? Java::JavaLang::IllegalArgumentException
       render_bad_request(error.cause.getMessage)
+    elsif error.cause.java_kind_of? Java::OrgSonarServerExceptions::UnauthorizedException
+      render_native_access_denied
+    elsif error.cause.java_kind_of? Java::OrgSonarServerExceptions::ForbiddenException
+      render_native_access_denied
     elsif error.cause.java_kind_of? Java::OrgSonarServerExceptions::HttpException
       render_server_exception(error.cause)
     else

sonar-server/src/main/webapp/WEB-INF/app/controllers/issues_controller.rb

@@ -79,15 +79,8 @@ class IssuesController < ApplicationController
   def save_as
     verify_post_request
     options = {'name' => params[:name], 'description' => params[:description], 'data' => URI.unescape(params[:data]), 'shared' => params[:shared]=='true'}
-    filter_result = Internal.issues.createIssueFilter(options)
-
-    if filter_result.ok
-      @filter = filter_result.get()
-      render :text => @filter.id.to_s, :status => 200
-    else
-      @errors = filter_result.errors
-      render :partial => 'issues/display_errors', :status => 400
-    end
+    @filter = Internal.issues.createIssueFilter(options)
+    render :text => @filter.id.to_s, :status => 200
   end
 
   # POST /issues/save?id=<id>&[criteria]
@@ -95,20 +88,8 @@ class IssuesController < ApplicationController
     verify_post_request
     require_parameters :id
 
-    filter_result = Internal.issues.updateIssueFilterQuery(params[:id].to_i, params)
-    if filter_result.ok
-      @filter = filter_result.get()
-      redirect_to :action => 'filter', :id => @filter.id.to_s
-    else
-      @unchanged = true
-      @errors = filter_result.errors
-
-      issue_filter_result = Internal.issues.execute(@filter.id, params)
-      @issue_query = issue_filter_result.query
-      @issues_result = issue_filter_result.result
-
-      render :action => 'search'
-    end
+    @filter = Internal.issues.updateIssueFilterQuery(params[:id].to_i, params)
+    redirect_to :action => 'filter', :id => @filter.id.to_s
   end
 
   # GET /issues/edit_form/<filter id>
@@ -125,14 +106,8 @@ class IssuesController < ApplicationController
     existing_filter = find_filter(params[:id].to_i)
     options = {'id' => params[:id].to_s, 'name' => params[:name], 'description' => params[:description],
                'data' => existing_filter.data, 'shared' => params[:shared]=='true', 'user' => params[:user]}
-    filter_result = Internal.issues.updateIssueFilter(options)
-    if filter_result.ok
-      @filter = filter_result.get()
-      render :text => @filter.id.to_s, :status => 200
-    else
-      @errors = filter_result.errors
-      render :partial => 'issues/display_errors', :status => 400
-    end
+    @filter = Internal.issues.updateIssueFilter(options)
+    render :text => @filter.id.to_s, :status => 200
   end
 
   # GET /issues/copy_form/<filter id>
@@ -147,15 +122,8 @@ class IssuesController < ApplicationController
     verify_post_request
 
     options = {'name' => params[:name], 'description' => params[:description], 'shared' => params[:shared]=='true'}
-    filter_result = Internal.issues.copyIssueFilter(params[:id].to_i, options)
-
-    if filter_result.ok
-      @filter = filter_result.get()
-      render :text => @filter.id.to_s, :status => 200
-    else
-      @errors = filter_result.errors
-      render :partial => 'issues/display_errors', :status => 400
-    end
+    @filter = Internal.issues.copyIssueFilter(params[:id].to_i, options)
+    render :text => @filter.id.to_s, :status => 200
   end
 
   # POST /issues/delete/<filter id>
@@ -170,13 +138,8 @@ class IssuesController < ApplicationController
   def toggle_fav
     verify_ajax_request
     require_parameters :id
-    result = Internal.issues.toggleFavouriteIssueFilter(params[:id].to_i)
-    if result.ok
-      render :text => '', :status => 200
-    else
-      @errors = result.errors
-      render :action => 'manage'
-    end
+    Internal.issues.toggleFavouriteIssueFilter(params[:id].to_i)
+    render :text => '', :status => 200
   end
 
   # GET /issues/bulk_change_form?[&criteria]
@@ -214,13 +177,8 @@ class IssuesController < ApplicationController
   # POST /issues/bulk_change
   def bulk_change
     verify_post_request
-    result = Internal.issues.bulkChange(params, params[:comment])
-    if result.ok
-      render :text => params[:criteria_params], :status => 200
-    else
-      @errors = result.errors
-      render :partial => 'issues/display_errors', :status => 400
-    end
+    Internal.issues.bulkChange(params, params[:comment])
+    render :text => params[:criteria_params], :status => 200
   end
 
 

sonar-server/src/main/webapp/WEB-INF/app/views/issues/_display_errors.html.erb

@@ -1,7 +0,0 @@
-<%
-   if @errors
-     @errors.each do |msg| %>
-    <div class="error"><%= h (msg.text ? msg.text : Api::Utils.message(msg.l10nKey, :params => msg.l10nParams)) -%></div>
-  <% end
-   end
-%>

sonar-server/src/main/webapp/WEB-INF/app/views/issues/_filter_shared_form.html.erb

@@ -2,7 +2,7 @@
      display_owner = false
    end %>
 <div class="modal-body">
-  <div class="issue-filter errors" style="display:none;" />
+  <div id="issue-filter-error" class="issue-filter errors error" style="display:none;" />
   <div class="modal-field">
     <label for="name"><%= message('issue_filter.form.name') -%> <em class="mandatory">*</em></label>
     <input id="name" name="name" type="text" size="50" maxlength="100" value="<%= h(@filter.name) if @filter -%>" autofocus="autofocus"/>

sonar-server/src/main/webapp/WEB-INF/app/views/issues/manage.html.erb

@@ -21,7 +21,6 @@
   </div>
   <div class="page-split-right">
     <div id="content">
-      <%= render :partial => 'display_errors' %>
       <h1><%= message 'issue_filter.manage.my_filters' -%></h1>
       <table class="data marginbottom10" id="my-issue-filters">
         <thead>

sonar-server/src/main/webapp/WEB-INF/app/views/issues/search.html.erb

@@ -5,8 +5,6 @@
 
   <div class="page-split-right">
     <div id="content">
-      <%= render :partial => 'display_errors' %>
-
       <div id="issue-filters-operations" class="line-block marginbottom10">
         <% if logged_in? && !@first_search %>
           <ul class="operations">

sonar-server/src/test/java/org/sonar/server/issue/InternalRubyIssueServiceTest.java

@@ -33,6 +33,7 @@ import org.sonar.core.issue.DefaultIssueFilter;
 import org.sonar.core.resource.ResourceDao;
 import org.sonar.core.resource.ResourceDto;
 import org.sonar.core.resource.ResourceQuery;
+import org.sonar.server.exceptions.BadRequestException;
 import org.sonar.server.user.UserSession;
 
 import java.util.Collections;
@@ -41,6 +42,7 @@ import java.util.Map;
 import static com.google.common.collect.Lists.newArrayList;
 import static com.google.common.collect.Maps.newHashMap;
 import static org.fest.assertions.Assertions.assertThat;
+import static org.fest.assertions.Fail.fail;
 import static org.mockito.Matchers.eq;
 import static org.mockito.Mockito.*;
 
@@ -298,36 +300,15 @@ public class InternalRubyIssueServiceTest {
     parameters.put("name", "Long term");
     parameters.put("description", "Long term issues");
 
-    Result<DefaultIssueFilter> result = service.createIssueFilter(parameters);
-    assertThat(result.ok()).isTrue();
+    service.createIssueFilter(parameters);
 
     ArgumentCaptor<DefaultIssueFilter> issueFilterCaptor = ArgumentCaptor.forClass(DefaultIssueFilter.class);
     verify(issueFilterService).save(issueFilterCaptor.capture(), any(UserSession.class));
-    DefaultIssueFilter issueFilter =  issueFilterCaptor.getValue();
+    DefaultIssueFilter issueFilter = issueFilterCaptor.getValue();
     assertThat(issueFilter.name()).isEqualTo("Long term");
     assertThat(issueFilter.description()).isEqualTo("Long term issues");
   }
 
-  @Test
-  public void should_not_create_issue_filter() {
-    Result result = service.createIssueFilter(Maps.<String, String>newHashMap());
-    assertThat(result.ok()).isFalse();
-    verify(issueFilterService, never()).save(any(DefaultIssueFilter.class), any(UserSession.class));
-  }
-
-  @Test
-  public void should_return_error_on_create_issue_filter() {
-    Map<String, String> parameters = newHashMap();
-    parameters.put("name", "Long term");
-    parameters.put("description", "Long term issues");
-    parameters.put("user", "John");
-
-    doThrow(new RuntimeException("Error")).when(issueFilterService).save(any(DefaultIssueFilter.class), any(UserSession.class));
-    Result result = service.createIssueFilter(parameters);
-    assertThat(result.ok()).isFalse();
-    assertThat(((Result.Message) result.errors().get(0)).text()).contains("Error");
-  }
-
   @Test
   public void should_update_issue_filter() {
     Map<String, String> parameters = newHashMap();
@@ -336,38 +317,16 @@ public class InternalRubyIssueServiceTest {
     parameters.put("description", "Long term issues");
     parameters.put("user", "John");
 
-    Result<DefaultIssueFilter> result = service.updateIssueFilter(parameters);
-    assertThat(result.ok()).isTrue();
+    service.updateIssueFilter(parameters);
 
     ArgumentCaptor<DefaultIssueFilter> issueFilterCaptor = ArgumentCaptor.forClass(DefaultIssueFilter.class);
     verify(issueFilterService).update(issueFilterCaptor.capture(), any(UserSession.class));
-    DefaultIssueFilter issueFilter =  issueFilterCaptor.getValue();
+    DefaultIssueFilter issueFilter = issueFilterCaptor.getValue();
     assertThat(issueFilter.id()).isEqualTo(10L);
     assertThat(issueFilter.name()).isEqualTo("Long term");
     assertThat(issueFilter.description()).isEqualTo("Long term issues");
   }
 
-  @Test
-  public void should_not_update_issue_filter() {
-    Result result = service.updateIssueFilter(Maps.<String, String>newHashMap());
-    assertThat(result.ok()).isFalse();
-    verify(issueFilterService, never()).update(any(DefaultIssueFilter.class), any(UserSession.class));
-  }
-
-  @Test
-  public void should_return_error_on_update_issue_filter() {
-    Map<String, String> parameters = newHashMap();
-    parameters.put("id", "10");
-    parameters.put("name", "Long term");
-    parameters.put("description", "Long term issues");
-    parameters.put("user", "John");
-
-    doThrow(new RuntimeException("Error")).when(issueFilterService).update(any(DefaultIssueFilter.class), any(UserSession.class));
-    Result result = service.updateIssueFilter(parameters);
-    assertThat(result.ok()).isFalse();
-    assertThat(((Result.Message) result.errors().get(0)).text()).contains("Error");
-  }
-
   @Test
   public void should_update_data() {
     Map<String, Object> data = newHashMap();
@@ -377,16 +336,8 @@ public class InternalRubyIssueServiceTest {
 
   @Test
   public void should_delete_issue_filter() {
-    Result<DefaultIssueFilter> result = service.deleteIssueFilter(1L);
-    assertThat(result.ok()).isTrue();
-  }
-
-  @Test
-  public void should_return_error_on_delete_issue_filter() {
-    doThrow(new RuntimeException("Error")).when(issueFilterService).delete(anyLong(), any(UserSession.class));
-    Result result = service.deleteIssueFilter(1L);
-    assertThat(result.ok()).isFalse();
-    assertThat(((Result.Message) result.errors().get(0)).text()).contains("Error");
+    service.deleteIssueFilter(1L);
+    verify(issueFilterService).delete(eq(1L), any(UserSession.class));
   }
 
   @Test
@@ -395,46 +346,29 @@ public class InternalRubyIssueServiceTest {
     parameters.put("name", "Copy of Long term");
     parameters.put("description", "Copy of Long term issues");
 
-    Result<DefaultIssueFilter> result = service.copyIssueFilter(1L, parameters);
-    assertThat(result.ok()).isTrue();
+    service.copyIssueFilter(1L, parameters);
 
     ArgumentCaptor<DefaultIssueFilter> issueFilterCaptor = ArgumentCaptor.forClass(DefaultIssueFilter.class);
     verify(issueFilterService).copy(eq(1L), issueFilterCaptor.capture(), any(UserSession.class));
-    DefaultIssueFilter issueFilter =  issueFilterCaptor.getValue();
+    DefaultIssueFilter issueFilter = issueFilterCaptor.getValue();
     assertThat(issueFilter.name()).isEqualTo("Copy of Long term");
     assertThat(issueFilter.description()).isEqualTo("Copy of Long term issues");
   }
 
-  @Test
-  public void should_not_copy_issue_filter() {
-    Result result = service.copyIssueFilter(1L, Maps.<String, String>newHashMap());
-    assertThat(result.ok()).isFalse();
-    verify(issueFilterService, never()).copy(anyLong(), any(DefaultIssueFilter.class), any(UserSession.class));
-  }
-
-  @Test
-  public void should_return_error_on_copy_issue_filter() {
-    Map<String, String> parameters = newHashMap();
-    parameters.put("name", "Copy of Long term");
-    parameters.put("description", "Copy of Long term issues");
-    parameters.put("user", "John");
-
-    doThrow(new RuntimeException("Error")).when(issueFilterService).copy(anyLong(), any(DefaultIssueFilter.class), any(UserSession.class));
-    Result result = service.copyIssueFilter(1L, parameters);
-    assertThat(result.ok()).isFalse();
-    assertThat(((Result.Message) result.errors().get(0)).text()).contains("Error");
-  }
-
   @Test
   public void should_get_error_on_create_issue_filter_result_when_no_name() {
     Map<String, String> parameters = newHashMap();
-    parameters.put("name", null);
+    parameters.put("name", "");
     parameters.put("description", "Long term issues");
     parameters.put("user", "John");
 
-    Result result = service.createIssueFilterResultForNew(parameters);
-    assertThat(result.ok()).isFalse();
-    assertThat(result.errors()).contains(Result.Message.ofL10n("errors.cant_be_empty", "name"));
+    try {
+      service.createIssueFilterResultForNew(parameters);
+      fail();
+    } catch (Exception e) {
+      assertThat(e).isInstanceOf(BadRequestException.class);
+      checkBadRequestException(e, "errors.cant_be_empty", "name");
+    }
   }
 
   @Test
@@ -444,9 +378,13 @@ public class InternalRubyIssueServiceTest {
     parameters.put("description", "Long term issues");
     parameters.put("user", "John");
 
-    Result result = service.createIssueFilterResultForNew(parameters);
-    assertThat(result.ok()).isFalse();
-    assertThat(result.errors()).contains(Result.Message.ofL10n("errors.is_too_long", "name", 100));
+    try {
+      service.createIssueFilterResultForNew(parameters);
+      fail();
+    } catch (Exception e) {
+      assertThat(e).isInstanceOf(BadRequestException.class);
+      checkBadRequestException(e, "errors.is_too_long", "name", 100);
+    }
   }
 
   @Test
@@ -456,9 +394,13 @@ public class InternalRubyIssueServiceTest {
     parameters.put("description", createLongString(4001));
     parameters.put("user", "John");
 
-    Result result = service.createIssueFilterResultForNew(parameters);
-    assertThat(result.ok()).isFalse();
-    assertThat(result.errors()).contains(Result.Message.ofL10n("errors.is_too_long", "description", 4000));
+    try {
+      service.createIssueFilterResultForNew(parameters);
+      fail();
+    } catch (Exception e) {
+      assertThat(e).isInstanceOf(BadRequestException.class);
+      checkBadRequestException(e, "errors.is_too_long", "description", 4000);
+    }
   }
 
   @Test
@@ -469,9 +411,13 @@ public class InternalRubyIssueServiceTest {
     parameters.put("description", "Long term issues");
     parameters.put("user", "John");
 
-    Result result = service.createIssueFilterResultForUpdate(parameters);
-    assertThat(result.ok()).isFalse();
-    assertThat(result.errors()).contains(Result.Message.ofL10n("errors.cant_be_empty", "id"));
+    try {
+      service.createIssueFilterResultForUpdate(parameters);
+      fail();
+    } catch (Exception e) {
+      assertThat(e).isInstanceOf(BadRequestException.class);
+      checkBadRequestException(e, "errors.cant_be_empty", "id");
+    }
   }
 
   @Test
@@ -482,9 +428,13 @@ public class InternalRubyIssueServiceTest {
     parameters.put("description", "Long term issues");
     parameters.put("user", null);
 
-    Result result = service.createIssueFilterResultForUpdate(parameters);
-    assertThat(result.ok()).isFalse();
-    assertThat(result.errors()).contains(Result.Message.ofL10n("errors.cant_be_empty", "user"));
+    try {
+      service.createIssueFilterResultForUpdate(parameters);
+      fail();
+    } catch (Exception e) {
+      assertThat(e).isInstanceOf(BadRequestException.class);
+      checkBadRequestException(e, "errors.cant_be_empty", "user");
+    }
   }
 
   @Test
@@ -495,8 +445,8 @@ public class InternalRubyIssueServiceTest {
     parameters.put("description", "Long term issues");
     parameters.put("user", null);
 
-    Result result = service.createIssueFilterResultForCopy(parameters);
-    assertThat(result.ok()).isTrue();
+    DefaultIssueFilter result = service.createIssueFilterResultForCopy(parameters);
+    assertThat(result).isNotNull();
   }
 
   @Test
@@ -546,7 +496,7 @@ public class InternalRubyIssueServiceTest {
   }
 
   @Test
-  public void should_sanitize_filter_query(){
+  public void should_sanitize_filter_query() {
     Map<String, Object> query = newHashMap();
     query.put("statuses", "CLOSED");
     query.put("resolved", true);
@@ -580,14 +530,6 @@ public class InternalRubyIssueServiceTest {
     verify(issueFilterService).toggleFavouriteIssueFilter(eq(10L), any(UserSession.class));
   }
 
-  @Test
-  public void should_return_error_on_toggle_favourite_issue_filter() {
-    doThrow(new RuntimeException("Error")).when(issueFilterService).toggleFavouriteIssueFilter(eq(10L), any(UserSession.class));
-    Result result = service.toggleFavouriteIssueFilter(10L);
-    assertThat(result.ok()).isFalse();
-    assertThat(((Result.Message) result.errors().get(0)).text()).contains("Error");
-  }
-
   @Test
   public void should_check_is_user_is_authorized_to_see_issue_filter() {
     DefaultIssueFilter issueFilter = new DefaultIssueFilter();
@@ -611,20 +553,12 @@ public class InternalRubyIssueServiceTest {
     verify(issueBulkChangeService).execute(any(IssueBulkChangeQuery.class), any(UserSession.class));
   }
 
-  @Test
-  public void should_no_execute_bulk_change_if_unexpected_error() {
-    doThrow(new RuntimeException("Error")).when(issueBulkChangeService).execute(any(IssueBulkChangeQuery.class), any(UserSession.class));
-
-    Map<String, Object> params = newHashMap();
-    params.put("issues", newArrayList("ABCD", "EFGH"));
-    params.put("actions", newArrayList("assign"));
-    params.put("assign.assignee", "arthur");
-    Result result = service.bulkChange(params, "A comment");
-    assertThat(result.ok()).isFalse();
-    assertThat(((Result.Message) result.errors().get(0)).text()).contains("Error");
+  private void checkBadRequestException(Exception e, String key, Object... params) {
+    BadRequestException exception = (BadRequestException) e;
+    assertThat(exception.l10nKey()).isEqualTo(key);
+    assertThat(exception.l10nParams()).containsOnly(params);
   }
 
-
   private String createLongString(int size) {
     String result = "";
     for (int i = 0; i < size; i++) {

sonar-server/src/test/java/org/sonar/server/issue/IssueFilterServiceTest.java

@@ -38,6 +38,10 @@ import org.sonar.core.issue.db.IssueFilterFavouriteDao;
 import org.sonar.core.issue.db.IssueFilterFavouriteDto;
 import org.sonar.core.user.AuthorizationDao;
 import org.sonar.core.user.Permission;
+import org.sonar.server.exceptions.BadRequestException;
+import org.sonar.server.exceptions.ForbiddenException;
+import org.sonar.server.exceptions.NotFoundException;
+import org.sonar.server.exceptions.UnauthorizedException;
 import org.sonar.server.user.MockUserSession;
 import org.sonar.server.user.UserSession;
 
@@ -54,11 +58,7 @@ import static org.mockito.Matchers.anyLong;
 import static org.mockito.Matchers.anyMap;
 import static org.mockito.Matchers.argThat;
 import static org.mockito.Matchers.eq;
-import static org.mockito.Mockito.mock;
-import static org.mockito.Mockito.never;
-import static org.mockito.Mockito.verify;
-import static org.mockito.Mockito.verifyZeroInteractions;
-import static org.mockito.Mockito.when;
+import static org.mockito.Mockito.*;
 
 public class IssueFilterServiceTest {
 
@@ -125,7 +125,7 @@ public class IssueFilterServiceTest {
       service.find(1L, userSession);
       fail();
     } catch (Exception e) {
-      assertThat(e).isInstanceOf(IllegalArgumentException.class).hasMessage("Filter not found: 1");
+      assertThat(e).isInstanceOf(NotFoundException.class).hasMessage("Filter not found: 1");
     }
   }
 
@@ -136,7 +136,7 @@ public class IssueFilterServiceTest {
       service.find(1L, userSession);
       fail();
     } catch (Exception e) {
-      assertThat(e).isInstanceOf(IllegalStateException.class).hasMessage("User is not logged in");
+      assertThat(e).isInstanceOf(UnauthorizedException.class).hasMessage("User is not logged in");
     }
     verifyZeroInteractions(issueFilterDao);
   }
@@ -150,7 +150,7 @@ public class IssueFilterServiceTest {
       service.find(1L, userSession);
       fail();
     } catch (Exception e) {
-      assertThat(e).isInstanceOf(IllegalStateException.class).hasMessage("User is not authorized to read this filter");
+      assertThat(e).isInstanceOf(ForbiddenException.class).hasMessage("User is not authorized to read this filter");
     }
   }
 
@@ -170,7 +170,7 @@ public class IssueFilterServiceTest {
       service.findByUser(userSession);
       fail();
     } catch (Exception e) {
-      assertThat(e).isInstanceOf(IllegalStateException.class).hasMessage("User is not logged in");
+      assertThat(e).isInstanceOf(UnauthorizedException.class).hasMessage("User is not logged in");
     }
   }
 
@@ -201,7 +201,7 @@ public class IssueFilterServiceTest {
       service.save(issueFilter, userSession);
       fail();
     } catch (Exception e) {
-      assertThat(e).isInstanceOf(IllegalStateException.class).hasMessage("User is not logged in");
+      assertThat(e).isInstanceOf(UnauthorizedException.class).hasMessage("User is not logged in");
     }
     verify(issueFilterDao, never()).insert(any(IssueFilterDto.class));
   }
@@ -214,7 +214,7 @@ public class IssueFilterServiceTest {
       service.save(issueFilter, userSession);
       fail();
     } catch (Exception e) {
-      assertThat(e).isInstanceOf(IllegalArgumentException.class).hasMessage("Name already exists");
+      assertThat(e).isInstanceOf(BadRequestException.class).hasMessage("Name already exists");
     }
     verify(issueFilterDao, never()).insert(any(IssueFilterDto.class));
   }
@@ -228,7 +228,7 @@ public class IssueFilterServiceTest {
       service.save(issueFilter, userSession);
       fail();
     } catch (Exception e) {
-      assertThat(e).isInstanceOf(IllegalArgumentException.class).hasMessage("Other users already share filters with the same name");
+      assertThat(e).isInstanceOf(BadRequestException.class).hasMessage("Other users already share filters with the same name");
     }
     verify(issueFilterDao, never()).insert(any(IssueFilterDto.class));
   }
@@ -290,7 +290,7 @@ public class IssueFilterServiceTest {
       service.update(new DefaultIssueFilter().setId(1L).setName("My New Filter"), userSession);
       fail();
     } catch (Exception e) {
-      assertThat(e).isInstanceOf(IllegalArgumentException.class).hasMessage("Filter not found: 1");
+      assertThat(e).isInstanceOf(NotFoundException.class).hasMessage("Filter not found: 1");
     }
     verify(issueFilterDao, never()).update(any(IssueFilterDto.class));
   }
@@ -304,7 +304,7 @@ public class IssueFilterServiceTest {
       service.update(new DefaultIssueFilter().setId(1L).setName("My New Filter"), userSession);
       fail();
     } catch (Exception e) {
-      assertThat(e).isInstanceOf(IllegalStateException.class).hasMessage("User is not authorized to modify this filter");
+      assertThat(e).isInstanceOf(ForbiddenException.class).hasMessage("User is not authorized to modify this filter");
     }
     verify(issueFilterDao, never()).update(any(IssueFilterDto.class));
   }
@@ -318,7 +318,7 @@ public class IssueFilterServiceTest {
       service.update(new DefaultIssueFilter().setId(1L).setUser("john").setName("My Issue"), userSession);
       fail();
     } catch (Exception e) {
-      assertThat(e).isInstanceOf(IllegalArgumentException.class).hasMessage("Name already exists");
+      assertThat(e).isInstanceOf(BadRequestException.class).hasMessage("Name already exists");
     }
     verify(issueFilterDao, never()).update(any(IssueFilterDto.class));
   }
@@ -366,7 +366,7 @@ public class IssueFilterServiceTest {
       service.update(issueFilter, MockUserSession.create().setUserId(1).setLogin(currentUser));
       fail();
     } catch (Exception e) {
-      assertThat(e).isInstanceOf(IllegalStateException.class).hasMessage("User is not authorized to change the owner of this filter");
+      assertThat(e).isInstanceOf(ForbiddenException.class).hasMessage("User is not authorized to change the owner of this filter");
     }
 
     verify(issueFilterDao, never()).update(any(IssueFilterDto.class));
@@ -400,7 +400,7 @@ public class IssueFilterServiceTest {
       service.delete(1L, userSession);
       fail();
     } catch (Exception e) {
-      assertThat(e).isInstanceOf(IllegalArgumentException.class).hasMessage("Filter not found: 1");
+      assertThat(e).isInstanceOf(NotFoundException.class).hasMessage("Filter not found: 1");
     }
     verify(issueFilterDao, never()).delete(anyLong());
   }
@@ -424,7 +424,7 @@ public class IssueFilterServiceTest {
       service.delete(1L, userSession);
       fail();
     } catch (Exception e) {
-      assertThat(e).isInstanceOf(IllegalStateException.class).hasMessage("User is not authorized to read this filter");
+      assertThat(e).isInstanceOf(ForbiddenException.class).hasMessage("User is not authorized to read this filter");
     }
     verify(issueFilterDao, never()).delete(anyLong());
   }
@@ -438,7 +438,7 @@ public class IssueFilterServiceTest {
       service.delete(1L, userSession);
       fail();
     } catch (Exception e) {
-      assertThat(e).isInstanceOf(IllegalStateException.class).hasMessage("User is not authorized to modify this filter");
+      assertThat(e).isInstanceOf(ForbiddenException.class).hasMessage("User is not authorized to modify this filter");
     }
     verify(issueFilterDao, never()).delete(anyLong());
   }
@@ -516,7 +516,7 @@ public class IssueFilterServiceTest {
       service.findFavoriteFilters(userSession);
       fail();
     } catch (Exception e) {
-      assertThat(e).isInstanceOf(IllegalStateException.class).hasMessage("User is not logged in");
+      assertThat(e).isInstanceOf(UnauthorizedException.class).hasMessage("User is not logged in");
     }
   }
 
@@ -567,7 +567,7 @@ public class IssueFilterServiceTest {
       service.toggleFavouriteIssueFilter(1L, userSession);
       fail();
     } catch (Exception e) {
-      assertThat(e).isInstanceOf(IllegalArgumentException.class).hasMessage("Filter not found: 1");
+      assertThat(e).isInstanceOf(NotFoundException.class).hasMessage("Filter not found: 1");
     }
     verify(issueFilterFavouriteDao, never()).delete(anyLong());
   }