diff options
12 files changed, 136 insertions, 31 deletions
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 @@ -69,6 +69,18 @@ public class RuleDaoTest extends AbstractDaoTestCase { } @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"); List<RuleDto> ruleDtos = dao.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); + } + } |