From e1af2f331051a08ea1f5dee369847340be85ad5e Mon Sep 17 00:00:00 2001 From: Julien Lancelot Date: Tue, 24 Dec 2013 10:10:55 +0100 Subject: [PATCH] SONAR-4535 Update rule now use Java facade --- .../java/org/sonar/core/rule/RuleDao.java | 20 ++++++- .../java/org/sonar/core/rule/RuleMapper.java | 2 + .../org/sonar/core/rule/RuleMapper.xml | 15 ++++-- .../java/org/sonar/core/rule/RuleDaoTest.java | 16 ++++++ .../RuleDaoTest/update_parameter-result.xml | 3 ++ .../rule/RuleDaoTest/update_parameter.xml | 3 ++ .../qualityprofile/QProfileOperations.java | 26 ++++++++- .../server/qualityprofile/QProfileRule.java | 5 ++ .../server/qualityprofile/QProfiles.java | 31 +++++++---- .../new_rules_configuration_controller.rb | 42 +++++++-------- .../new_rules_configuration/edit.html.erb | 20 +++---- .../QProfileOperationsTest.java | 24 ++++++++- .../server/qualityprofile/QProfilesTest.java | 54 +++++++++++++++++-- 13 files changed, 208 insertions(+), 53 deletions(-) create mode 100644 sonar-core/src/test/resources/org/sonar/core/rule/RuleDaoTest/update_parameter-result.xml create mode 100644 sonar-core/src/test/resources/org/sonar/core/rule/RuleDaoTest/update_parameter.xml 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 482835a57b6..922efb2c782 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 @@ -72,10 +72,14 @@ public class RuleDao implements BatchComponent, ServerComponent { } } + public void update(RuleDto rule, SqlSession session) { + getMapper(session).update(rule); + } + public void update(RuleDto rule) { SqlSession session = mybatis.openSession(); try { - getMapper(session).update(rule); + update(rule, session); session.commit(); } finally { MyBatis.closeQuietly(session); @@ -144,6 +148,20 @@ public class RuleDao implements BatchComponent, ServerComponent { } } + public void update(RuleParamDto param, SqlSession session) { + getMapper(session).updateParameter(param); + } + + public void update(RuleParamDto param) { + SqlSession session = mybatis.openSession(); + try { + update(param, session); + session.commit(); + } finally { + MyBatis.closeQuietly(session); + } + } + @CheckForNull public RuleParamDto selectParamByRuleAndKey(Integer ruleId, String key, SqlSession session) { return getMapper(session).selectParamByRuleAndKey(ruleId, key); 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 9b2698d9453..8c340b33569 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 @@ -45,4 +45,6 @@ public interface RuleMapper { RuleParamDto selectParamByRuleAndKey(@Param("ruleId") Integer ruleId, @Param("key") String key); void insertParameter(RuleParamDto param); + + void updateParameter(RuleParamDto param); } 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 efa9a1f7290..95063093b9a 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 @@ -41,7 +41,7 @@ - update rules set + UPDATE rules SET plugin_rule_key=#{ruleKey}, plugin_name=#{repositoryKey}, description=#{description}, @@ -57,7 +57,7 @@ note_user_login=#{noteUserLogin}, note_created_at=#{noteCreatedAt}, note_updated_at=#{noteUpdatedAt} - where id=#{id} + WHERE id=#{id} @@ -105,10 +105,19 @@ - + INSERT INTO rules_parameters (rule_id, name, param_type, default_value, description) VALUES (#{ruleId}, #{name}, #{type}, #{defaultValue}, #{description}) + + UPDATE rules_parameters SET + name=#{name}, + param_type=#{type}, + default_value=#{defaultValue}, + description=#{description} + WHERE id=#{id} + + 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 f58e4790a15..43e0b9ce660 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 @@ -272,4 +272,20 @@ public class RuleDaoTest extends AbstractDaoTestCase { checkTables("insert_parameter", "rules_parameters"); } + + @Test + public void update_parameter() { + setupData("update_parameter"); + + RuleParamDto param = new RuleParamDto() + .setId(1) + .setName("format") + .setType("STRING") + .setDefaultValue("^[a-z]+(\\.[a-z][a-z0-9]*)*$") + .setDescription("Regular expression used to check the package names against."); + + dao.update(param); + + checkTables("update_parameter", "rules_parameters"); + } } diff --git a/sonar-core/src/test/resources/org/sonar/core/rule/RuleDaoTest/update_parameter-result.xml b/sonar-core/src/test/resources/org/sonar/core/rule/RuleDaoTest/update_parameter-result.xml new file mode 100644 index 00000000000..d61889441d9 --- /dev/null +++ b/sonar-core/src/test/resources/org/sonar/core/rule/RuleDaoTest/update_parameter-result.xml @@ -0,0 +1,3 @@ + + + diff --git a/sonar-core/src/test/resources/org/sonar/core/rule/RuleDaoTest/update_parameter.xml b/sonar-core/src/test/resources/org/sonar/core/rule/RuleDaoTest/update_parameter.xml new file mode 100644 index 00000000000..5208b7a4a4c --- /dev/null +++ b/sonar-core/src/test/resources/org/sonar/core/rule/RuleDaoTest/update_parameter.xml @@ -0,0 +1,3 @@ + + + diff --git a/sonar-server/src/main/java/org/sonar/server/qualityprofile/QProfileOperations.java b/sonar-server/src/main/java/org/sonar/server/qualityprofile/QProfileOperations.java index 6d6f23de554..0a64684dbdd 100644 --- a/sonar-server/src/main/java/org/sonar/server/qualityprofile/QProfileOperations.java +++ b/sonar-server/src/main/java/org/sonar/server/qualityprofile/QProfileOperations.java @@ -379,7 +379,7 @@ public class QProfileOperations implements ServerComponent { .setName(key) .setDescription(templateRuleParam.getDescription()) .setType(templateRuleParam.getType()) - .setDefaultValue(value); + .setDefaultValue(Strings.emptyToNull(value)); ruleDao.insert(param, session); ruleParams.add(param); } @@ -392,6 +392,30 @@ public class QProfileOperations implements ServerComponent { } } + public void updateRule(RuleDto rule, String name, String severity, String description, Map paramsByKey, + UserSession userSession) { + checkPermission(userSession); + SqlSession session = myBatis.openSession(); + try { + rule.setName(name) + .setDescription(description) + .setSeverity(Severity.ordinal(severity)) + .setUpdatedAt(new Date(system.now())); + ruleDao.update(rule, session); + + List ruleParams = ruleDao.selectParameters(rule.getId(), session); + for (RuleParamDto ruleParam : ruleParams) { + String value = paramsByKey.get(ruleParam.getName()); + ruleParam.setDefaultValue(Strings.emptyToNull(value)); + ruleDao.update(ruleParam, session); + } + session.commit(); + ruleRegistry.save(rule, ruleParams); + } finally { + MyBatis.closeQuietly(session); + } + } + private void reindexInheritanceResult(RuleInheritanceActions actions, SqlSession session) { ruleRegistry.deleteActiveRules(actions.idsToDelete()); List activeRules = activeRuleDao.selectByIds(actions.idsToIndex(), session); diff --git a/sonar-server/src/main/java/org/sonar/server/qualityprofile/QProfileRule.java b/sonar-server/src/main/java/org/sonar/server/qualityprofile/QProfileRule.java index 4d7baac1ad5..1cf94932a04 100644 --- a/sonar-server/src/main/java/org/sonar/server/qualityprofile/QProfileRule.java +++ b/sonar-server/src/main/java/org/sonar/server/qualityprofile/QProfileRule.java @@ -184,6 +184,11 @@ public class QProfileRule { return inheritance; } + @CheckForNull + public Integer parentId() { + return parentId; + } + public boolean isInherited() { return ActiveRule.INHERITED.equals(inheritance); } 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 d857fd283b8..42096d410db 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 @@ -303,13 +303,24 @@ public class QProfiles implements ServerComponent { return rules.getFromRuleId(ruleId); } - public QProfileRule newRule(int ruleId, @Nullable String name, @Nullable String severity, @Nullable String description, Map paramsByKey) { + public QProfileRule createRule(int ruleId, @Nullable String name, @Nullable String severity, @Nullable String description, Map paramsByKey) { RuleDto rule = findRuleNotNull(ruleId); - validateNewRule(name, severity, description); + validateRule(null, name, severity, description); RuleDto newRule = operations.createRule(rule, name, severity, description, paramsByKey, UserSession.get()); return rules.getFromRuleId(newRule.getId()); } + public QProfileRule updateRule(int ruleId, @Nullable String name, @Nullable String severity, @Nullable String description, Map paramsByKey) { + RuleDto rule = findRuleNotNull(ruleId); + if (rule.getParentId() == null) { + throw new NotFoundException("Unknown rule"); + } + validateRule(ruleId, name, severity, description); + operations.updateRule(rule, name, severity, description, paramsByKey, UserSession.get()); + return rules.getFromRuleId(ruleId); + } + + // // Quality profile validation // @@ -385,17 +396,17 @@ public class QProfiles implements ServerComponent { // Rule validation // - private void validateNewRule(@Nullable String name, @Nullable String severity, @Nullable String description) { + private void validateRule(@Nullable Integer updatingRuleId, @Nullable String name, @Nullable String severity, @Nullable String description) { List messages = newArrayList(); - if (Strings.isNullOrEmpty(name)){ + if (Strings.isNullOrEmpty(name)) { messages.add(BadRequestException.Message.ofL10n(Validation.CANT_BE_EMPTY_MESSAGE, "Name")); } else { - checkRuleNotAlreadyExists(name, messages); + checkRuleNotAlreadyExists(updatingRuleId, name, messages); } - if (Strings.isNullOrEmpty(description)){ + if (Strings.isNullOrEmpty(description)) { messages.add(BadRequestException.Message.ofL10n(Validation.CANT_BE_EMPTY_MESSAGE, "Description")); } - if (Strings.isNullOrEmpty(severity)){ + if (Strings.isNullOrEmpty(severity)) { messages.add(BadRequestException.Message.ofL10n(Validation.CANT_BE_EMPTY_MESSAGE, "Severity")); } if (!messages.isEmpty()) { @@ -403,8 +414,10 @@ public class QProfiles implements ServerComponent { } } - private void checkRuleNotAlreadyExists(String name, List messages) { - if (ruleDao.selectByName(name) != null) { + private void checkRuleNotAlreadyExists(@Nullable Integer updatingRuleId, String name, List messages) { + RuleDto existingRule = ruleDao.selectByName(name); + boolean isModifyingCurrentRule = updatingRuleId != null && existingRule != null && existingRule.getId().equals(updatingRuleId); + if (!isModifyingCurrentRule && existingRule != null) { messages.add(BadRequestException.Message.ofL10n(Validation.IS_ALREADY_USED_MESSAGE, "Name")); } } diff --git a/sonar-server/src/main/webapp/WEB-INF/app/controllers/new_rules_configuration_controller.rb b/sonar-server/src/main/webapp/WEB-INF/app/controllers/new_rules_configuration_controller.rb index 9d97816221c..6b1b0abb6fa 100644 --- a/sonar-server/src/main/webapp/WEB-INF/app/controllers/new_rules_configuration_controller.rb +++ b/sonar-server/src/main/webapp/WEB-INF/app/controllers/new_rules_configuration_controller.rb @@ -164,7 +164,7 @@ class NewRulesConfigurationController < ApplicationController rule_id = params[:rule_id].to_i new_rule = nil call_backend do - new_rule = Internal.quality_profiles.newRule(rule_id, params[:rule][:name], params[:rule][:priority], params[:rule][:description], params[:rule_param]) + new_rule = Internal.quality_profiles.createRule(rule_id, params[:rule][:name], params[:rule][:priority], params[:rule][:description], params[:rule_param]) end if new_rule @@ -187,12 +187,14 @@ class NewRulesConfigurationController < ApplicationController # def edit # form to edit a rule - access_denied unless has_role?(:profileadmin) require_parameters :id, :rule_id - @profile = Profile.find(params[:id]) - @rule = Rule.find(params[:rule_id]) - if !@rule.editable? + + @profile = Internal.quality_profiles.profile(params[:id].to_i) + @rule = Internal.quality_profiles.rule(params[:rule_id].to_i) + if @rule.parentId().nil? redirect_to :action => 'index', :id => params[:id] + else + @parent_rule = Internal.quality_profiles.rule(@rule.parentId()) end end @@ -203,27 +205,19 @@ class NewRulesConfigurationController < ApplicationController # def update verify_post_request - access_denied unless has_role?(:profileadmin) require_parameters :id, :rule_id - rule=Rule.find(params[:rule_id]) - if rule.editable? - rule.name=params[:rule][:name] - rule.description=params[:rule][:description] - rule.priority=Sonar::RulePriority.id(params[:rule][:priority]) - rule.parameters.each do |parameter| - parameter.default_value=params[:rule_param][parameter.name] - parameter.save - end - if rule.save - Internal.rules.saveOrUpdate(rule.id) - redirect_to :action => 'index', :id => params[:id], :searchtext => "\"#{rule.name}\"", :rule_activation => '', "plugins[]" => rule.plugin_name - else - flash[:error]=message('rules_configuration.rule_not_valid_message_x', :params => rule.errors.full_messages.join('
')) - redirect_to :action => 'new', :id => params[:id], :rule_id => params[:rule_id] - end + + profile_id = params[:id].to_i + rule_id = params[:rule_id].to_i + rule = nil + call_backend do + rule = Internal.quality_profiles.updateRule(rule_id, params[:rule][:name], params[:rule][:priority], params[:rule][:description], params[:rule_param]) + end + + if rule + redirect_to :action => 'index', :id => profile_id, :searchtext => "\"#{rule.name()}\"", :rule_activation => '', "plugins[]" => rule.repositoryKey() else - flash[:error]='Unknown rule' - redirect_to :action => 'index', :id => params[:id] + redirect_to :action => 'new', :id => profile_id, :rule_id => rule_id end end diff --git a/sonar-server/src/main/webapp/WEB-INF/app/views/new_rules_configuration/edit.html.erb b/sonar-server/src/main/webapp/WEB-INF/app/views/new_rules_configuration/edit.html.erb index fba58000636..94812eb5d13 100644 --- a/sonar-server/src/main/webapp/WEB-INF/app/views/new_rules_configuration/edit.html.erb +++ b/sonar-server/src/main/webapp/WEB-INF/app/views/new_rules_configuration/edit.html.erb @@ -18,34 +18,34 @@ return false; } -
+ - + - + - <% @rule.parameters.sort{|x,y| x.name <=> y.name}.each do |parameter| %> + <% @rule.params.to_a.sort{|x,y| x.key() <=> y.key()}.each do |parameter| %> - + <% end %> @@ -53,7 +53,7 @@ @@ -61,7 +61,7 @@
<%= message('template') -%>: <%= h @rule.parent.name -%><%= h @parent_rule.name() -%>
<%= message('name') -%>:
<%= message('default_severity') -%>:
<%= parameter.name %>:<%= parameter.key() %>: - <%= param_value_input(parameter, "#{h parameter.default_value}", {:name => "rule_param[#{h parameter.name }]", :size => '80x10'}) -%> - <%= h parameter.description %> + <%= param_value_input(@rule, parameter, "#{h parameter.defaultValue()}", {:name => "rule_param[#{h parameter.key() }]", :size => '80x10'}) -%> + <%= h parameter.description() %>
Description: - + <%= message('rules_configuration.html_allowed') -%>
- <%= message('cancel') -%> + <%= message('cancel') -%>
diff --git a/sonar-server/src/test/java/org/sonar/server/qualityprofile/QProfileOperationsTest.java b/sonar-server/src/test/java/org/sonar/server/qualityprofile/QProfileOperationsTest.java index 551e18cf816..b1f976c282a 100644 --- a/sonar-server/src/test/java/org/sonar/server/qualityprofile/QProfileOperationsTest.java +++ b/sonar-server/src/test/java/org/sonar/server/qualityprofile/QProfileOperationsTest.java @@ -540,7 +540,7 @@ public class QProfileOperationsTest { } @Test - public void create_new_rule() throws Exception { + public void create_rule() throws Exception { RuleDto templateRule = new RuleDto().setId(10).setRepositoryKey("squid").setRuleKey("AvoidCycle").setConfigKey("Xpath"); when(ruleDao.selectParameters(eq(10), eq(session))).thenReturn(newArrayList(new RuleParamDto().setId(20).setName("max").setDefaultValue("10"))); @@ -569,4 +569,26 @@ public class QProfileOperationsTest { verify(ruleRegistry).save(eq(ruleArgument.getValue()), eq(newArrayList(ruleParamArgument.getValue()))); } + @Test + public void update_rule() throws Exception { + RuleDto rule = new RuleDto().setId(11).setRepositoryKey("squid").setRuleKey("XPath_1387869254").setConfigKey("Xpath"); + when(ruleDao.selectParameters(eq(11), eq(session))).thenReturn(newArrayList(new RuleParamDto().setId(21).setName("max").setDefaultValue("20"))); + + Map paramsByKey = ImmutableMap.of("max", "21"); + operations.updateRule(rule, "Updated Rule", Severity.MAJOR, "Updated Description", paramsByKey, authorizedUserSession); + + ArgumentCaptor ruleArgument = ArgumentCaptor.forClass(RuleDto.class); + verify(ruleDao).update(ruleArgument.capture(), eq(session)); + assertThat(ruleArgument.getValue().getName()).isEqualTo("Updated Rule"); + assertThat(ruleArgument.getValue().getDescription()).isEqualTo("Updated Description"); + assertThat(ruleArgument.getValue().getSeverity()).isEqualTo(2); + + ArgumentCaptor ruleParamArgument = ArgumentCaptor.forClass(RuleParamDto.class); + verify(ruleDao).update(ruleParamArgument.capture(), eq(session)); + assertThat(ruleParamArgument.getValue().getDefaultValue()).isEqualTo("21"); + + verify(session).commit(); + verify(ruleRegistry).save(eq(ruleArgument.getValue()), eq(newArrayList(ruleParamArgument.getValue()))); + } + } 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 36ef43327f7..c2d2d960162 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 @@ -545,7 +545,7 @@ public class QProfilesTest { Map paramsByKey = ImmutableMap.of("max", "20"); when(service.createRule(eq(rule), eq("Rule name"), eq(Severity.MAJOR), eq("My note"), eq(paramsByKey), any(UserSession.class))).thenReturn(newRule); - qProfiles.newRule(10, "Rule name", Severity.MAJOR, "My note", paramsByKey); + qProfiles.createRule(10, "Rule name", Severity.MAJOR, "My note", paramsByKey); verify(service).createRule(eq(rule), eq("Rule name"), eq(Severity.MAJOR), eq("My note"), eq(paramsByKey), any(UserSession.class)); verify(rules).getFromRuleId(11); @@ -561,7 +561,7 @@ public class QProfilesTest { when(service.createRule(eq(rule), eq("Rule name"), eq(Severity.MAJOR), eq("My note"), eq(paramsByKey), any(UserSession.class))).thenReturn(newRule); try { - qProfiles.newRule( 10, "", "", "", paramsByKey); + qProfiles.createRule( 10, "", "", "", paramsByKey); fail(); } catch (Exception e) { assertThat(e).isInstanceOf(BadRequestException.class); @@ -580,10 +580,10 @@ public class QProfilesTest { RuleDto newRule = new RuleDto().setId(11); Map paramsByKey = ImmutableMap.of("max", "20"); - when(service.createRule(eq(rule), eq("Rule name"), eq(Severity.MAJOR), eq("My note"), eq(paramsByKey), any(UserSession.class))).thenReturn(newRule); + when(service.createRule(eq(rule), eq("Rule name"), eq(Severity.MAJOR), eq("My description"), eq(paramsByKey), any(UserSession.class))).thenReturn(newRule); try { - qProfiles.newRule(10, "Rule name", Severity.MAJOR, "My note", paramsByKey); + qProfiles.createRule(10, "Rule name", Severity.MAJOR, "My description", paramsByKey); fail(); } catch (Exception e) { assertThat(e).isInstanceOf(BadRequestException.class); @@ -593,4 +593,50 @@ public class QProfilesTest { verifyZeroInteractions(rules); } + @Test + public void update_rule() throws Exception { + RuleDto rule = new RuleDto().setId(11).setRepositoryKey("squid").setRuleKey("XPath_1387869254").setParentId(10); + when(ruleDao.selectById(11)).thenReturn(rule); + when(ruleDao.selectByName("Updated name")).thenReturn(null); + + Map paramsByKey = ImmutableMap.of("max", "21"); + + qProfiles.updateRule(11, "Updated name", Severity.MAJOR, "Updated description", paramsByKey); + + verify(service).updateRule(eq(rule), eq("Updated name"), eq(Severity.MAJOR), eq("Updated description"), eq(paramsByKey), any(UserSession.class)); + verify(rules).getFromRuleId(11); + } + + @Test + public void update_rule_with_same_name() throws Exception { + RuleDto rule = new RuleDto().setId(11).setRepositoryKey("squid").setRuleKey("XPath_1387869254").setParentId(10); + when(ruleDao.selectById(11)).thenReturn(rule); + when(ruleDao.selectByName("Rule name")).thenReturn(rule); + + Map paramsByKey = ImmutableMap.of("max", "21"); + + qProfiles.updateRule(11, "Rule name", Severity.MAJOR, "Updated description", paramsByKey); + + verify(service).updateRule(eq(rule), eq("Rule name"), eq(Severity.MAJOR), eq("Updated description"), eq(paramsByKey), any(UserSession.class)); + verify(rules).getFromRuleId(11); + } + + @Test + public void fail_to_update_rule_when_no_parent() throws Exception { + RuleDto rule = new RuleDto().setId(11).setRepositoryKey("squid").setRuleKey("XPath_1387869254"); + when(ruleDao.selectById(11)).thenReturn(rule); + when(ruleDao.selectByName("Rule name")).thenReturn(rule); + + Map paramsByKey = ImmutableMap.of("max", "21"); + + try { + qProfiles.updateRule(11, "Rule name", Severity.MAJOR, "Updated description", paramsByKey); + fail(); + } catch (Exception e) { + assertThat(e).isInstanceOf(NotFoundException.class); + } + verifyZeroInteractions(service); + verifyZeroInteractions(rules); + } + } -- 2.39.5