From 53fdf23671b1ae1ebf1541705ab49d6c30f72bd4 Mon Sep 17 00:00:00 2001 From: Godin Date: Tue, 21 Dec 2010 02:34:10 +0000 Subject: [PATCH] SONAR-1722: Support multiple levels of inheritance --- .../server/configuration/ProfilesManager.java | 57 ++++++++++++++----- .../rules_configuration_controller.rb | 1 - .../views/rules_configuration/index.html.erb | 14 ++--- .../configuration/InheritedProfilesTest.java | 22 +++++++ .../shouldActivateInChildren-result.xml | 4 +- .../shouldActivateInChildren.xml | 2 +- .../shouldChangeParent-result.xml | 6 +- .../shouldChangeParent.xml | 6 +- .../shouldCheckCycles.xml | 12 ++++ .../shouldDeactivateInChildren-result.xml | 2 +- .../shouldDeactivateInChildren.xml | 4 +- .../shouldRemoveParent-result.xml | 2 +- .../shouldRemoveParent.xml | 4 +- .../shouldSetParent-result.xml | 4 +- .../InheritedProfilesTest/shouldSetParent.xml | 2 +- 15 files changed, 99 insertions(+), 43 deletions(-) create mode 100644 sonar-server/src/test/resources/org/sonar/server/configuration/InheritedProfilesTest/shouldCheckCycles.xml diff --git a/sonar-server/src/main/java/org/sonar/server/configuration/ProfilesManager.java b/sonar-server/src/main/java/org/sonar/server/configuration/ProfilesManager.java index 9053b287aca..5a91e742b6b 100644 --- a/sonar-server/src/main/java/org/sonar/server/configuration/ProfilesManager.java +++ b/sonar-server/src/main/java/org/sonar/server/configuration/ProfilesManager.java @@ -71,14 +71,15 @@ public class ProfilesManager extends BaseDao { } // Managing inheritance of profiles - // Only one level of inheritance supported public void changeParentProfile(Integer profileId, String parentName) { RulesProfile profile = getSession().getEntity(RulesProfile.class, profileId); - // TODO check cycles if (profile != null && !profile.getProvided()) { RulesProfile oldParent = getParentProfile(profile); RulesProfile newParent = getProfile(profile.getLanguage(), parentName); + if (isCycle(profile, newParent)) { + return; + } // Deactivate all inherited rules if (oldParent != null) { for (ActiveRule activeRule : oldParent.getActiveRules()) { @@ -90,7 +91,6 @@ public class ProfilesManager extends BaseDao { for (ActiveRule activeRule : newParent.getActiveRules()) { activateOrChange(profile, activeRule); } - } else { } profile.setParentName(newParent == null ? null : newParent.getName()); getSession().saveWithoutFlush(profile); @@ -102,13 +102,12 @@ public class ProfilesManager extends BaseDao { * Rule was activated/changed in parent profile. */ public void activatedOrChanged(int parentProfileId, int activeRuleId) { - List children = getChildren(parentProfileId); ActiveRule parentActiveRule = getSession().getEntity(ActiveRule.class, activeRuleId); if (parentActiveRule.isInherited() && !parentActiveRule.isOverridden()) { parentActiveRule.setOverridden(true); getSession().saveWithoutFlush(parentActiveRule); } - for (RulesProfile child : children) { + for (RulesProfile child : getChildren(parentProfileId)) { activateOrChange(child, parentActiveRule); } getSession().commit(); @@ -118,14 +117,26 @@ public class ProfilesManager extends BaseDao { * Rule was deactivated in parent profile. */ public void deactivated(int parentProfileId, int ruleId) { - List children = getChildren(parentProfileId); Rule rule = getSession().getEntity(Rule.class, ruleId); - for (RulesProfile child : children) { + for (RulesProfile child : getChildren(parentProfileId)) { deactivate(child, rule); } getSession().commit(); } + /** + * @return true, if setting childProfile as a child of profile adds cycle + */ + boolean isCycle(RulesProfile childProfile, RulesProfile profile) { + while (profile != null) { + if (childProfile.equals(profile)) { + return true; + } + profile = getParentProfile(profile); + } + return false; + } + public void revert(int profileId, int activeRuleId) { RulesProfile profile = getSession().getEntity(RulesProfile.class, profileId); ActiveRule activeRule = getSession().getEntity(ActiveRule.class, activeRuleId); @@ -135,7 +146,13 @@ public class ProfilesManager extends BaseDao { activeRule = (ActiveRule) parentActiveRule.clone(); activeRule.setRulesProfile(profile); activeRule.setInherited(true); + activeRule.setOverridden(false); profile.getActiveRules().add(activeRule); + + for (RulesProfile child : getChildren(profile)) { + activateOrChange(child, activeRule); + } + getSession().commit(); } } @@ -149,13 +166,18 @@ public class ProfilesManager extends BaseDao { activeRule.setInherited(true); activeRule.setOverridden(true); getSession().saveWithoutFlush(activeRule); - return; + return; // no need to change in children } } activeRule = (ActiveRule) parentActiveRule.clone(); activeRule.setRulesProfile(profile); activeRule.setInherited(true); + activeRule.setOverridden(false); profile.getActiveRules().add(activeRule); + + for (RulesProfile child : getChildren(profile)) { + activateOrChange(child, activeRule); + } } private void deactivate(RulesProfile profile, Rule rule) { @@ -167,12 +189,21 @@ public class ProfilesManager extends BaseDao { activeRule.setInherited(false); activeRule.setOverridden(false); getSession().saveWithoutFlush(activeRule); + return; // no need to change in children + } + + for (RulesProfile child : getChildren(profile)) { + deactivate(child, rule); } } } private List getChildren(int parentId) { - RulesProfile parent = getProfile(parentId); + RulesProfile parent = getSession().getEntity(RulesProfile.class, parentId); + return getChildren(parent); + } + + private List getChildren(RulesProfile parent) { return getSession().getResults(RulesProfile.class, "language", parent.getLanguage(), "parentName", parent.getName(), @@ -184,17 +215,13 @@ public class ProfilesManager extends BaseDao { getSession().removeWithoutFlush(activeRule); } - private RulesProfile getProfile(Integer id) { - return id == null ? null : getSession().getEntity(RulesProfile.class, id); - } - - private RulesProfile getProfile(String language, String name) { + RulesProfile getProfile(String language, String name) { return getSession().getSingleResult(RulesProfile.class, "language", language, "name", name); } - private RulesProfile getParentProfile(RulesProfile profile) { + RulesProfile getParentProfile(RulesProfile profile) { if (profile.getParentName() == null) { return null; } diff --git a/sonar-server/src/main/webapp/WEB-INF/app/controllers/rules_configuration_controller.rb b/sonar-server/src/main/webapp/WEB-INF/app/controllers/rules_configuration_controller.rb index ddeb9a1fa20..fbb53a18eeb 100644 --- a/sonar-server/src/main/webapp/WEB-INF/app/controllers/rules_configuration_controller.rb +++ b/sonar-server/src/main/webapp/WEB-INF/app/controllers/rules_configuration_controller.rb @@ -55,7 +55,6 @@ class RulesConfigurationController < ApplicationController @select_priority = ANY_SELECTION + RULE_PRIORITIES @select_status = [['Any',''], ["Active", STATUS_ACTIVE], ["Inactive", STATUS_INACTIVE]] - @child_profiles = RulesProfile.find(:all, :conditions => {:language => @profile.language, :parent_name => @profile.name}) @select_parent = [['', nil]] + RulesProfile.find(:all).collect { |profile| [profile.name, profile.name] }.sort @rules = Rule.search(java_facade, { diff --git a/sonar-server/src/main/webapp/WEB-INF/app/views/rules_configuration/index.html.erb b/sonar-server/src/main/webapp/WEB-INF/app/views/rules_configuration/index.html.erb index 8a502bb3341..a68fc7968d0 100644 --- a/sonar-server/src/main/webapp/WEB-INF/app/views/rules_configuration/index.html.erb +++ b/sonar-server/src/main/webapp/WEB-INF/app/views/rules_configuration/index.html.erb @@ -69,16 +69,12 @@
- <% if @child_profiles.size == 0 %> - <% if !@profile.provided? %> - <% form_tag({:action => 'change_parent'}, {:method => 'post'}) do %> - <%= hidden_field_tag "id", @id %> - Parent profile: <%= select_tag "parent_name", options_for_select(@select_parent, @profile.parent_name), :disabled => !enable_modification %> - <%= submit_tag "Change", :id => 'submit_parent', :disabled => !enable_modification %> - <% end %> + <% if !@profile.provided? %> + <% form_tag({:action => 'change_parent'}, {:method => 'post'}) do %> + <%= hidden_field_tag "id", @id %> + Parent profile: <%= select_tag "parent_name", options_for_select(@select_parent, @profile.parent_name), :disabled => !enable_modification %> + <%= submit_tag "Change", :id => 'submit_parent', :disabled => !enable_modification %> <% end %> - <% else %> - This profile inherited by others. <% end %>
diff --git a/sonar-server/src/test/java/org/sonar/server/configuration/InheritedProfilesTest.java b/sonar-server/src/test/java/org/sonar/server/configuration/InheritedProfilesTest.java index 6d146700f2d..beaee7eb60c 100644 --- a/sonar-server/src/test/java/org/sonar/server/configuration/InheritedProfilesTest.java +++ b/sonar-server/src/test/java/org/sonar/server/configuration/InheritedProfilesTest.java @@ -2,8 +2,13 @@ package org.sonar.server.configuration; import org.junit.Before; import org.junit.Test; +import org.sonar.api.profiles.RulesProfile; import org.sonar.jpa.test.AbstractDbUnitTestCase; +import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.nullValue; +import static org.junit.Assert.assertThat; + public class InheritedProfilesTest extends AbstractDbUnitTestCase { private ProfilesManager profilesManager; @@ -12,6 +17,23 @@ public class InheritedProfilesTest extends AbstractDbUnitTestCase { profilesManager = new ProfilesManager(getSession(), null); } + @Test + public void shouldCheckCycles() { + setupData("shouldCheckCycles"); + RulesProfile level1 = profilesManager.getProfile("java", "level1"); + RulesProfile level2 = profilesManager.getProfile("java", "level2"); + RulesProfile level3 = profilesManager.getProfile("java", "level3"); + + assertThat(profilesManager.getParentProfile(level1), nullValue()); + assertThat(profilesManager.getParentProfile(level2), is(level1)); + assertThat(profilesManager.getParentProfile(level3), is(level2)); + + assertThat(profilesManager.isCycle(level1, level1), is(true)); + assertThat(profilesManager.isCycle(level1, level3), is(true)); + assertThat(profilesManager.isCycle(level1, level2), is(true)); + assertThat(profilesManager.isCycle(level2, level3), is(true)); + } + @Test public void shouldSetParent() { setupData("shouldSetParent"); diff --git a/sonar-server/src/test/resources/org/sonar/server/configuration/InheritedProfilesTest/shouldActivateInChildren-result.xml b/sonar-server/src/test/resources/org/sonar/server/configuration/InheritedProfilesTest/shouldActivateInChildren-result.xml index 36f0a7035e0..7a4d2a592b8 100644 --- a/sonar-server/src/test/resources/org/sonar/server/configuration/InheritedProfilesTest/shouldActivateInChildren-result.xml +++ b/sonar-server/src/test/resources/org/sonar/server/configuration/InheritedProfilesTest/shouldActivateInChildren-result.xml @@ -9,10 +9,10 @@ - + - + diff --git a/sonar-server/src/test/resources/org/sonar/server/configuration/InheritedProfilesTest/shouldActivateInChildren.xml b/sonar-server/src/test/resources/org/sonar/server/configuration/InheritedProfilesTest/shouldActivateInChildren.xml index 8eaee098614..019b5bb6bb3 100644 --- a/sonar-server/src/test/resources/org/sonar/server/configuration/InheritedProfilesTest/shouldActivateInChildren.xml +++ b/sonar-server/src/test/resources/org/sonar/server/configuration/InheritedProfilesTest/shouldActivateInChildren.xml @@ -9,7 +9,7 @@ - + diff --git a/sonar-server/src/test/resources/org/sonar/server/configuration/InheritedProfilesTest/shouldChangeParent-result.xml b/sonar-server/src/test/resources/org/sonar/server/configuration/InheritedProfilesTest/shouldChangeParent-result.xml index 2d8dcd06fef..f887123534a 100644 --- a/sonar-server/src/test/resources/org/sonar/server/configuration/InheritedProfilesTest/shouldChangeParent-result.xml +++ b/sonar-server/src/test/resources/org/sonar/server/configuration/InheritedProfilesTest/shouldChangeParent-result.xml @@ -12,10 +12,10 @@ - + - + - + diff --git a/sonar-server/src/test/resources/org/sonar/server/configuration/InheritedProfilesTest/shouldChangeParent.xml b/sonar-server/src/test/resources/org/sonar/server/configuration/InheritedProfilesTest/shouldChangeParent.xml index 4e94e54fa5f..7c6bbdb1ab8 100644 --- a/sonar-server/src/test/resources/org/sonar/server/configuration/InheritedProfilesTest/shouldChangeParent.xml +++ b/sonar-server/src/test/resources/org/sonar/server/configuration/InheritedProfilesTest/shouldChangeParent.xml @@ -12,10 +12,10 @@ - + - + - + diff --git a/sonar-server/src/test/resources/org/sonar/server/configuration/InheritedProfilesTest/shouldCheckCycles.xml b/sonar-server/src/test/resources/org/sonar/server/configuration/InheritedProfilesTest/shouldCheckCycles.xml new file mode 100644 index 00000000000..fd3b5e3e589 --- /dev/null +++ b/sonar-server/src/test/resources/org/sonar/server/configuration/InheritedProfilesTest/shouldCheckCycles.xml @@ -0,0 +1,12 @@ + + + + + + + + + + + diff --git a/sonar-server/src/test/resources/org/sonar/server/configuration/InheritedProfilesTest/shouldDeactivateInChildren-result.xml b/sonar-server/src/test/resources/org/sonar/server/configuration/InheritedProfilesTest/shouldDeactivateInChildren-result.xml index 874f79d03e4..393ca0e3a7c 100644 --- a/sonar-server/src/test/resources/org/sonar/server/configuration/InheritedProfilesTest/shouldDeactivateInChildren-result.xml +++ b/sonar-server/src/test/resources/org/sonar/server/configuration/InheritedProfilesTest/shouldDeactivateInChildren-result.xml @@ -7,6 +7,6 @@ - + diff --git a/sonar-server/src/test/resources/org/sonar/server/configuration/InheritedProfilesTest/shouldDeactivateInChildren.xml b/sonar-server/src/test/resources/org/sonar/server/configuration/InheritedProfilesTest/shouldDeactivateInChildren.xml index 016e11a46f4..9aa8741732b 100644 --- a/sonar-server/src/test/resources/org/sonar/server/configuration/InheritedProfilesTest/shouldDeactivateInChildren.xml +++ b/sonar-server/src/test/resources/org/sonar/server/configuration/InheritedProfilesTest/shouldDeactivateInChildren.xml @@ -7,8 +7,8 @@ - + - + diff --git a/sonar-server/src/test/resources/org/sonar/server/configuration/InheritedProfilesTest/shouldRemoveParent-result.xml b/sonar-server/src/test/resources/org/sonar/server/configuration/InheritedProfilesTest/shouldRemoveParent-result.xml index 783ad6580b8..c7d7a39b4a8 100644 --- a/sonar-server/src/test/resources/org/sonar/server/configuration/InheritedProfilesTest/shouldRemoveParent-result.xml +++ b/sonar-server/src/test/resources/org/sonar/server/configuration/InheritedProfilesTest/shouldRemoveParent-result.xml @@ -7,6 +7,6 @@ - + diff --git a/sonar-server/src/test/resources/org/sonar/server/configuration/InheritedProfilesTest/shouldRemoveParent.xml b/sonar-server/src/test/resources/org/sonar/server/configuration/InheritedProfilesTest/shouldRemoveParent.xml index 016e11a46f4..9aa8741732b 100644 --- a/sonar-server/src/test/resources/org/sonar/server/configuration/InheritedProfilesTest/shouldRemoveParent.xml +++ b/sonar-server/src/test/resources/org/sonar/server/configuration/InheritedProfilesTest/shouldRemoveParent.xml @@ -7,8 +7,8 @@ - + - + diff --git a/sonar-server/src/test/resources/org/sonar/server/configuration/InheritedProfilesTest/shouldSetParent-result.xml b/sonar-server/src/test/resources/org/sonar/server/configuration/InheritedProfilesTest/shouldSetParent-result.xml index 016e11a46f4..9aa8741732b 100644 --- a/sonar-server/src/test/resources/org/sonar/server/configuration/InheritedProfilesTest/shouldSetParent-result.xml +++ b/sonar-server/src/test/resources/org/sonar/server/configuration/InheritedProfilesTest/shouldSetParent-result.xml @@ -7,8 +7,8 @@ - + - + diff --git a/sonar-server/src/test/resources/org/sonar/server/configuration/InheritedProfilesTest/shouldSetParent.xml b/sonar-server/src/test/resources/org/sonar/server/configuration/InheritedProfilesTest/shouldSetParent.xml index 783ad6580b8..c7d7a39b4a8 100644 --- a/sonar-server/src/test/resources/org/sonar/server/configuration/InheritedProfilesTest/shouldSetParent.xml +++ b/sonar-server/src/test/resources/org/sonar/server/configuration/InheritedProfilesTest/shouldSetParent.xml @@ -7,6 +7,6 @@ - + -- 2.39.5