SONAR-4535 Add some check when creating new rule

6 files changed, 102 insertions, 30 deletions

diff --git a/sonar-server/src/main/java/org/sonar/server/exceptions/BadRequestException.java b/sonar-server/src/main/java/org/sonar/server/exceptions/BadRequestException.java
index e0bff2fafbb..424aa57408c 100644
--- a/sonar-server/src/main/java/org/sonar/server/exceptions/BadRequestException.java
+++ b/sonar-server/src/main/java/org/sonar/server/exceptions/BadRequestException.java
@@ -23,7 +23,6 @@ import org.apache.commons.lang.builder.ReflectionToStringBuilder;
 
 import javax.annotation.CheckForNull;
 import javax.annotation.Nullable;
-
 import java.util.Arrays;
 import java.util.List;
 
@@ -60,7 +59,7 @@ public class BadRequestException extends ServerException {
     return new BadRequestException(message);
   }
 
-  public static BadRequestException of(String message, List<Message> errors) {
+  public static BadRequestException of(@Nullable  String message, List<Message> errors) {
     return new BadRequestException(message, errors);
   }
 
diff --git a/sonar-server/src/main/java/org/sonar/server/issue/InternalRubyIssueService.java b/sonar-server/src/main/java/org/sonar/server/issue/InternalRubyIssueService.java
index a27fd1891dc..cf7a8b6320f 100644
--- a/sonar-server/src/main/java/org/sonar/server/issue/InternalRubyIssueService.java
+++ b/sonar-server/src/main/java/org/sonar/server/issue/InternalRubyIssueService.java
@@ -51,7 +51,6 @@ import org.sonar.server.util.Validation;
 
 import javax.annotation.CheckForNull;
 import javax.annotation.Nullable;
-
 import java.util.Collection;
 import java.util.Date;
 import java.util.List;
@@ -353,7 +352,7 @@ public class InternalRubyIssueService implements ServerComponent {
 
   private void checkProject(String projectParam, Result<ActionPlan> result) {
     if (Strings.isNullOrEmpty(projectParam)) {
-      result.addError(Result.Message.ofL10n(Validation.ERRORS_CANT_BE_EMPTY_MESSAGE, PROJECT_PARAM));
+      result.addError(Result.Message.ofL10n(Validation.CANT_BE_EMPTY_MESSAGE, PROJECT_PARAM));
     } else {
       ResourceDto project = resourceDao.getResource(ResourceQuery.create().setKey(projectParam));
       if (project == null) {
@@ -594,26 +593,26 @@ public class InternalRubyIssueService implements ServerComponent {
 
   private void checkMandatoryParameter(String value, String paramName, Result result) {
     if (Strings.isNullOrEmpty(value)) {
-      result.addError(Result.Message.ofL10n(Validation.ERRORS_CANT_BE_EMPTY_MESSAGE, paramName));
+      result.addError(Result.Message.ofL10n(Validation.CANT_BE_EMPTY_MESSAGE, paramName));
     }
   }
 
   private void checkMandatorySizeParameter(String value, String paramName, Integer size, Result result) {
     checkMandatoryParameter(value, paramName, result);
     if (!Strings.isNullOrEmpty(value) && value.length() > size) {
-      result.addError(Result.Message.ofL10n(Validation.ERRORS_IS_TOO_LONG_MESSAGE, paramName, size));
+      result.addError(Result.Message.ofL10n(Validation.IS_TOO_LONG_MESSAGE, paramName, size));
     }
   }
 
   private void checkOptionalSizeParameter(String value, String paramName, Integer size, Result result) {
     if (!Strings.isNullOrEmpty(value) && value.length() > size) {
-      result.addError(Result.Message.ofL10n(Validation.ERRORS_IS_TOO_LONG_MESSAGE, paramName, size));
+      result.addError(Result.Message.ofL10n(Validation.IS_TOO_LONG_MESSAGE, paramName, size));
     }
   }
 
   private void checkOptionalSizeParameter(String value, String paramName, Integer size) {
     if (!Strings.isNullOrEmpty(value) && value.length() > size) {
-      throw BadRequestException.ofL10n(Validation.ERRORS_IS_TOO_LONG_MESSAGE, paramName, size);
+      throw BadRequestException.ofL10n(Validation.IS_TOO_LONG_MESSAGE, paramName, size);
     }
   }
 
diff --git a/sonar-server/src/main/java/org/sonar/server/qualityprofile/QProfiles.java b/sonar-server/src/main/java/org/sonar/server/qualityprofile/QProfiles.java
index 81f4042861a..82b62182a8f 100644
--- a/sonar-server/src/main/java/org/sonar/server/qualityprofile/QProfiles.java
+++ b/sonar-server/src/main/java/org/sonar/server/qualityprofile/QProfiles.java
@@ -37,10 +37,11 @@ import org.sonar.server.util.Validation;
 
 import javax.annotation.CheckForNull;
 import javax.annotation.Nullable;
-
 import java.util.List;
 import java.util.Map;
 
+import static com.google.common.collect.Lists.newArrayList;
+
 /**
  * Used through ruby code <pre>Internal.quality_profiles</pre>
  */
@@ -101,7 +102,7 @@ public class QProfiles implements ServerComponent {
 
   @CheckForNull
   public QProfile parent(QProfile profile) {
-    QualityProfileDto parent = find(profile.parent(), profile.language());
+    QualityProfileDto parent = findQualityProfile(profile.parent(), profile.language());
     if (parent != null) {
       return QProfile.from(parent);
     }
@@ -185,7 +186,7 @@ public class QProfiles implements ServerComponent {
 
   /**
    * Used to load ancestor active rule of an active rule
-   *
+   * <p/>
    * TODO Ancestor active rules should be integrated into QProfileRule or elsewhere in order to load all ancestor active rules once a time
    */
   @CheckForNull
@@ -235,7 +236,7 @@ public class QProfiles implements ServerComponent {
 
   /**
    * Used to load ancestor param of an active rule param
-   *
+   * <p/>
    * TODO Ancestor params should be integrated into QProfileRuleParam or elsewhere in order to load all ancestor params once a time
    */
   @CheckForNull
@@ -297,11 +298,10 @@ public class QProfiles implements ServerComponent {
     return rules.getFromRuleId(ruleId);
   }
 
-  public QProfileRule newRule(int profileId, int ruleId,  String name, String severity, String description, Map<String, String> paramsByKey) {
+  public QProfileRule newRule(int profileId, int ruleId, @Nullable String name, @Nullable String severity, @Nullable String description, Map<String, String> paramsByKey) {
     QualityProfileDto qualityProfile = findNotNull(profileId);
     RuleDto rule = findRuleNotNull(ruleId);
-
-    // TODO fail if empty name, description, severity (in one error)
+    validateNewRule(name, severity, description);
     RuleDto newRule = operations.createRule(qualityProfile, rule, name, severity, description, paramsByKey, UserSession.get());
     return rules.getFromRuleId(newRule.getId());
   }
@@ -320,25 +320,24 @@ public class QProfiles implements ServerComponent {
     validateProfileName(newName);
     QualityProfileDto profileDto = findNotNull(profileId);
     if (!profileDto.getName().equals(newName)) {
-      // TODO move this check to the service
       checkNotAlreadyExists(newName, profileDto.getLanguage());
     }
     return profileDto;
   }
 
   private void checkNotAlreadyExists(String name, String language) {
-    if (find(name, language) != null) {
+    if (findQualityProfile(name, language) != null) {
       throw BadRequestException.ofL10n("quality_profiles.already_exists");
     }
   }
 
   private QualityProfileDto findNotNull(int id) {
-    QualityProfileDto qualityProfile = find(id);
+    QualityProfileDto qualityProfile = findQualityProfile(id);
     return checkNotNull(qualityProfile);
   }
 
   private QualityProfileDto findNotNull(String name, String language) {
-    QualityProfileDto qualityProfile = find(name, language);
+    QualityProfileDto qualityProfile = findQualityProfile(name, language);
     return checkNotNull(qualityProfile);
   }
 
@@ -350,12 +349,12 @@ public class QProfiles implements ServerComponent {
   }
 
   @CheckForNull
-  private QualityProfileDto find(String name, String language) {
+  private QualityProfileDto findQualityProfile(String name, String language) {
     return qualityProfileDao.selectByNameAndLanguage(name, language);
   }
 
   @CheckForNull
-  private QualityProfileDto find(int id) {
+  private QualityProfileDto findQualityProfile(int id) {
     return qualityProfileDao.selectById(id);
   }
 
@@ -377,10 +376,35 @@ public class QProfiles implements ServerComponent {
     return component;
   }
 
+
   //
   // Rule validation
   //
 
+  private void validateNewRule(@Nullable String name, @Nullable String severity, @Nullable String description) {
+    List<BadRequestException.Message> messages = newArrayList();
+    if (Strings.isNullOrEmpty(name)){
+      messages.add(BadRequestException.Message.ofL10n(Validation.CANT_BE_EMPTY_MESSAGE, "Name"));
+    } else {
+      checkRuleNotAlreadyExists(name, messages);
+    }
+    if (Strings.isNullOrEmpty(description)){
+      messages.add(BadRequestException.Message.ofL10n(Validation.CANT_BE_EMPTY_MESSAGE, "Description"));
+    }
+    if (Strings.isNullOrEmpty(severity)){
+      messages.add(BadRequestException.Message.ofL10n(Validation.CANT_BE_EMPTY_MESSAGE, "Severity"));
+    }
+    if (!messages.isEmpty()) {
+      throw new BadRequestException(null, messages);
+    }
+  }
+
+  private void checkRuleNotAlreadyExists(String name, List<BadRequestException.Message> messages) {
+    if (ruleDao.selectByName(name) != null) {
+      messages.add(BadRequestException.Message.ofL10n(Validation.IS_ALREADY_USED_MESSAGE, "Name"));
+    }
+  }
+
   private RuleDto findRuleNotNull(int ruleId) {
     RuleDto rule = ruleDao.selectById(ruleId);
     if (rule == null) {
@@ -424,7 +448,7 @@ public class QProfiles implements ServerComponent {
     return activeRuleDao.selectParamByActiveRuleAndKey(activeRuleId, key);
   }
 
-  private ActiveRuleChanged activeRuleChanged(QualityProfileDto qualityProfile, ActiveRuleDto activeRule){
+  private ActiveRuleChanged activeRuleChanged(QualityProfileDto qualityProfile, ActiveRuleDto activeRule) {
     return new ActiveRuleChanged(QProfile.from(qualityProfile), rules.getFromActiveRuleId(activeRule.getId()));
   }
 
diff --git a/sonar-server/src/main/java/org/sonar/server/util/Validation.java b/sonar-server/src/main/java/org/sonar/server/util/Validation.java
index d99898f6304..c6f87c03d0b 100644
--- a/sonar-server/src/main/java/org/sonar/server/util/Validation.java
+++ b/sonar-server/src/main/java/org/sonar/server/util/Validation.java
@@ -24,8 +24,9 @@ import org.sonar.server.exceptions.BadRequestException;
 
 public class Validation {
 
-  public static final String ERRORS_CANT_BE_EMPTY_MESSAGE = "errors.cant_be_empty";
-  public static final String ERRORS_IS_TOO_LONG_MESSAGE = "errors.is_too_long";
+  public static final String CANT_BE_EMPTY_MESSAGE = "errors.cant_be_empty";
+  public static final String IS_TOO_LONG_MESSAGE = "errors.is_too_long";
+  public static final String IS_ALREADY_USED_MESSAGE = "errors.is_already_used";
 
   private Validation() {
     // only static methods
@@ -33,20 +34,20 @@ public class Validation {
 
   public static void checkMandatoryParameter(String value, String paramName) {
     if (Strings.isNullOrEmpty(value)) {
-      throw BadRequestException.ofL10n(Validation.ERRORS_CANT_BE_EMPTY_MESSAGE, paramName);
+      throw BadRequestException.ofL10n(Validation.CANT_BE_EMPTY_MESSAGE, paramName);
     }
   }
 
   public static void checkMandatoryParameter(Object value, String paramName) {
     if (value == null) {
-      throw BadRequestException.ofL10n(Validation.ERRORS_CANT_BE_EMPTY_MESSAGE, paramName);
+      throw BadRequestException.ofL10n(Validation.CANT_BE_EMPTY_MESSAGE, paramName);
     }
   }
 
   public static void checkMandatorySizeParameter(String value, String paramName, Integer size) {
     checkMandatoryParameter(value, paramName);
     if (!Strings.isNullOrEmpty(value) && value.length() > size) {
-      throw BadRequestException.ofL10n(Validation.ERRORS_IS_TOO_LONG_MESSAGE, paramName, size);
+      throw BadRequestException.ofL10n(Validation.IS_TOO_LONG_MESSAGE, paramName, size);
     }
   }
 
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 64bcc347291..4c7e8018e36 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
@@ -228,9 +228,12 @@ class ApplicationController < ActionController::Base
   end
 
   def java_error_message(exception)
-    message = (exception.getMessage ? exception.getMessage : Api::Utils.message(exception.l10nKey, :params => exception.l10nParams.to_a))
-    if exception.java_kind_of?(Java::OrgSonarServerExceptions::BadRequestException) && !exception.errors.empty?
-      message += '<br/>' + exception.errors.to_a.map{|error| error.text ? error.text : Api::Utils.message(error.l10nKey, :params => error.l10nParams)}.join('<br/>')
+    message = ''
+    message += (exception.getMessage ? exception.getMessage : Api::Utils.message(exception.l10nKey, :params => exception.l10nParams.to_a)) if exception.getMessage or exception.l10nKey
+    has_errors = exception.java_kind_of?(Java::OrgSonarServerExceptions::BadRequestException) && !exception.errors.empty?
+    message += '<br/>' unless message.blank? or !has_errors
+    if has_errors
+      message += exception.errors.to_a.map{|error| error.text ? error.text : Api::Utils.message(error.l10nKey, :params => error.l10nParams)}.join('<br/>')
     end
     message
   end
diff --git a/sonar-server/src/test/java/org/sonar/server/qualityprofile/QProfilesTest.java b/sonar-server/src/test/java/org/sonar/server/qualityprofile/QProfilesTest.java
index fa6fbdae569..26f88f31c37 100644
--- a/sonar-server/src/test/java/org/sonar/server/qualityprofile/QProfilesTest.java
+++ b/sonar-server/src/test/java/org/sonar/server/qualityprofile/QProfilesTest.java
@@ -553,4 +553,50 @@ public class QProfilesTest {
     verify(rules).getFromRuleId(11);
   }
 
+  @Test
+  public void fail_to_create_new_rule_on_empty_parameters() throws Exception {
+    QualityProfileDto profile = new QualityProfileDto().setId(1).setName("My profile").setLanguage("java");
+    when(qualityProfileDao.selectById(1)).thenReturn(profile);
+    RuleDto rule = new RuleDto().setId(10).setRepositoryKey("squid").setRuleKey("AvoidCycle");
+    when(ruleDao.selectById(10)).thenReturn(rule);
+
+    RuleDto newRule = new RuleDto().setId(11);
+    Map<String, String> paramsByKey = ImmutableMap.of("max", "20");
+    when(service.createRule(eq(profile), eq(rule), eq("Rule name"), eq(Severity.MAJOR), eq("My note"), eq(paramsByKey), any(UserSession.class))).thenReturn(newRule);
+
+    try {
+      qProfiles.newRule(1, 10, "", "", "", paramsByKey);
+      fail();
+    } catch (Exception e) {
+      assertThat(e).isInstanceOf(BadRequestException.class);
+      assertThat(((BadRequestException) e).errors()).hasSize(3);
+    }
+    verifyZeroInteractions(service);
+    verifyZeroInteractions(rules);
+  }
+
+  @Test
+  public void fail_to_create_new_rule_when_rule_name_already_exists() throws Exception {
+    QualityProfileDto profile = new QualityProfileDto().setId(1).setName("My profile").setLanguage("java");
+    when(qualityProfileDao.selectById(1)).thenReturn(profile);
+    RuleDto rule = new RuleDto().setId(10).setRepositoryKey("squid").setRuleKey("AvoidCycle");
+    when(ruleDao.selectById(10)).thenReturn(rule);
+
+    when(ruleDao.selectByName("Rule name")).thenReturn(new RuleDto());
+
+    RuleDto newRule = new RuleDto().setId(11);
+    Map<String, String> paramsByKey = ImmutableMap.of("max", "20");
+    when(service.createRule(eq(profile), eq(rule), eq("Rule name"), eq(Severity.MAJOR), eq("My note"), eq(paramsByKey), any(UserSession.class))).thenReturn(newRule);
+
+    try {
+      qProfiles.newRule(1, 10, "Rule name", Severity.MAJOR, "My note", paramsByKey);
+      fail();
+    } catch (Exception e) {
+      assertThat(e).isInstanceOf(BadRequestException.class);
+      assertThat(((BadRequestException) e).errors()).hasSize(1);
+    }
+    verifyZeroInteractions(service);
+    verifyZeroInteractions(rules);
+  }
+
 }