From 5e87ef770b0195d7abfbcf071232940ae14588b5 Mon Sep 17 00:00:00 2001
From: Julien Lancelot <julien.lancelot@gmail.com>
Date: Mon, 23 Dec 2013 22:36:03 +0100
Subject: [PATCH] SONAR-4535 Add some check when creating new rule

---
 .../resources/org/sonar/l10n/core.properties  |  1 +
 .../java/org/sonar/core/rule/RuleDao.java     | 10 +++-
 .../java/org/sonar/core/rule/RuleMapper.java  |  2 +
 .../org/sonar/core/rule/RuleMapper.xml        |  4 ++
 .../java/org/sonar/core/rule/RuleDaoTest.java | 12 +++++
 .../core/rule/RuleDaoTest/select_by_name.xml  |  6 +++
 .../exceptions/BadRequestException.java       |  3 +-
 .../issue/InternalRubyIssueService.java       | 11 ++--
 .../server/qualityprofile/QProfiles.java      | 52 ++++++++++++++-----
 .../org/sonar/server/util/Validation.java     | 11 ++--
 .../app/controllers/application_controller.rb |  9 ++--
 .../server/qualityprofile/QProfilesTest.java  | 46 ++++++++++++++++
 12 files changed, 136 insertions(+), 31 deletions(-)
 create mode 100644 sonar-core/src/test/resources/org/sonar/core/rule/RuleDaoTest/select_by_name.xml

diff --git a/plugins/sonar-core-plugin/src/main/resources/org/sonar/l10n/core.properties b/plugins/sonar-core-plugin/src/main/resources/org/sonar/l10n/core.properties
index e9f259b70c4..a384edfb355 100644
--- a/plugins/sonar-core-plugin/src/main/resources/org/sonar/l10n/core.properties
+++ b/plugins/sonar-core-plugin/src/main/resources/org/sonar/l10n/core.properties
@@ -2486,6 +2486,7 @@ permission_template.default_for=Default for {0}
 #------------------------------------------------------------------------------
 errors.is_too_short={0} is too short (minimum is {1} characters)
 errors.is_too_long={0} is too long (maximum is {1} characters)
+errors.is_already_used={0} has already been taken
 errors.cant_be_empty={0} can't be empty
 errors.is_not_valid={0} is not valid
 
diff --git a/sonar-core/src/main/java/org/sonar/core/rule/RuleDao.java b/sonar-core/src/main/java/org/sonar/core/rule/RuleDao.java
index c768b85e0ad..482835a57b6 100644
--- a/sonar-core/src/main/java/org/sonar/core/rule/RuleDao.java
+++ b/sonar-core/src/main/java/org/sonar/core/rule/RuleDao.java
@@ -25,7 +25,6 @@ import org.sonar.api.ServerComponent;
 import org.sonar.core.persistence.MyBatis;
 
 import javax.annotation.CheckForNull;
-
 import java.util.Collection;
 import java.util.List;
 
@@ -64,6 +63,15 @@ public class RuleDao implements BatchComponent, ServerComponent {
     }
   }
 
+  public RuleDto selectByName(String name) {
+    SqlSession session = mybatis.openSession();
+    try {
+      return getMapper(session).selectByName(name);
+    } finally {
+      MyBatis.closeQuietly(session);
+    }
+  }
+
   public void update(RuleDto rule) {
     SqlSession session = mybatis.openSession();
     try {
diff --git a/sonar-core/src/main/java/org/sonar/core/rule/RuleMapper.java b/sonar-core/src/main/java/org/sonar/core/rule/RuleMapper.java
index 405220f7fab..9b2698d9453 100644
--- a/sonar-core/src/main/java/org/sonar/core/rule/RuleMapper.java
+++ b/sonar-core/src/main/java/org/sonar/core/rule/RuleMapper.java
@@ -30,6 +30,8 @@ public interface RuleMapper {
 
   RuleDto selectById(Integer id);
 
+  RuleDto selectByName(String name);
+
   void update(RuleDto rule);
 
   void batchInsert(RuleDto rule);
diff --git a/sonar-core/src/main/resources/org/sonar/core/rule/RuleMapper.xml b/sonar-core/src/main/resources/org/sonar/core/rule/RuleMapper.xml
index 96243fe2028..efa9a1f7290 100644
--- a/sonar-core/src/main/resources/org/sonar/core/rule/RuleMapper.xml
+++ b/sonar-core/src/main/resources/org/sonar/core/rule/RuleMapper.xml
@@ -31,6 +31,10 @@
     select <include refid="selectColumns"/> from rules WHERE id=#{id}
   </select>
 
+  <select id="selectByName" parameterType="String" resultType="Rule">
+    select <include refid="selectColumns"/> from rules WHERE name=#{name}
+  </select>
+
   <select id="selectNonManual" resultType="Rule">
     select <include refid="selectColumns"/> from rules
     where plugin_name != 'manual'
diff --git a/sonar-core/src/test/java/org/sonar/core/rule/RuleDaoTest.java b/sonar-core/src/test/java/org/sonar/core/rule/RuleDaoTest.java
index 825ec610de8..f58e4790a15 100644
--- a/sonar-core/src/test/java/org/sonar/core/rule/RuleDaoTest.java
+++ b/sonar-core/src/test/java/org/sonar/core/rule/RuleDaoTest.java
@@ -68,6 +68,18 @@ public class RuleDaoTest extends AbstractDaoTestCase {
     assertThat(ruleDto.getRepositoryKey()).isEqualTo("checkstyle");
   }
 
+  @Test
+  public void select_by_name() throws Exception {
+    setupData("select_by_name");
+    RuleDto ruleDto = dao.selectByName("Avoid Null");
+
+    assertThat(ruleDto.getId()).isEqualTo(2);
+    assertThat(ruleDto.getName()).isEqualTo("Avoid Null");
+    assertThat(ruleDto.getDescription()).isEqualTo("Should avoid NULL");
+    assertThat(ruleDto.getStatus()).isEqualTo(Rule.STATUS_READY);
+    assertThat(ruleDto.getRepositoryKey()).isEqualTo("checkstyle");
+  }
+
   @Test
   public void testSelectNonManual() throws Exception {
     setupData("selectNonManual");
diff --git a/sonar-core/src/test/resources/org/sonar/core/rule/RuleDaoTest/select_by_name.xml b/sonar-core/src/test/resources/org/sonar/core/rule/RuleDaoTest/select_by_name.xml
new file mode 100644
index 00000000000..dc8fe1e2615
--- /dev/null
+++ b/sonar-core/src/test/resources/org/sonar/core/rule/RuleDaoTest/select_by_name.xml
@@ -0,0 +1,6 @@
+<dataset>
+
+  <rules id="1" plugin_rule_key="AvoidComparison" plugin_name="checkstyle" name="Avoid Comparison" description="Should avoid ==" status="READY"/>
+  <rules id="2" plugin_rule_key="AvoidNull" plugin_name="checkstyle" name="Avoid Null" description="Should avoid NULL" status="READY"/>
+
+</dataset>
\ No newline at end of file
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);
+  }
+
 }
-- 
2.39.5