SONAR-1722: Support multiple levels of inheritance

15 files changed, 99 insertions, 43 deletions

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<RulesProfile> 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<RulesProfile> 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<RulesProfile> getChildren(int parentId) {
-    RulesProfile parent = getProfile(parentId);
+    RulesProfile parent = getSession().getEntity(RulesProfile.class, parentId);
+    return getChildren(parent);
+  }
+
+  private List<RulesProfile> 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 @@
 </div>
 <div class="line-block marginbottom10">
   <div>
-    <% 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 %>
   </div>
 
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;
 
@@ -13,6 +18,23 @@ public class InheritedProfilesTest extends AbstractDbUnitTestCase {
   }
 
   @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");
     profilesManager.changeParentProfile(2, "parent");
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 @@
   
   <rules_profiles id="2" provided="false" name="child" default_profile="0" language="java" parent_name="parent"/>
 
-  <active_rules id="1" profile_id="1" rule_id="1" failure_level="2" inherited="false" overridden="[null]"/>
+  <active_rules id="1" profile_id="1" rule_id="1" failure_level="2" inherited="false" overridden="false"/>
   <active_rule_parameters id="1" active_rule_id="1" rules_parameter_id="1" value="30"/>
   
-  <active_rules id="2" profile_id="2" rule_id="1" failure_level="2" inherited="true" overridden="[null]"/>
+  <active_rules id="2" profile_id="2" rule_id="1" failure_level="2" inherited="true" overridden="false"/>
   <active_rule_parameters id="2" active_rule_id="2" rules_parameter_id="1" value="30"/>
   
 </dataset>
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 @@
   
   <rules_profiles id="2" provided="false" name="child" default_profile="0" language="java" parent_name="parent"/>
 
-  <active_rules id="1" profile_id="1" rule_id="1" failure_level="2" inherited="false" overridden="[null]"/>
+  <active_rules id="1" profile_id="1" rule_id="1" failure_level="2" inherited="false" overridden="false"/>
   <active_rule_parameters id="1" active_rule_id="1" rules_parameter_id="1" value="30"/>
 
 </dataset>
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 @@
   
   <rules_profiles id="3" provided="false" name="child" default_profile="0" language="java" parent_name="new_parent"/>
 
-  <active_rules id="1" profile_id="1" rule_id="1" failure_level="2" inherited="false" overridden="[null]"/>
+  <active_rules id="1" profile_id="1" rule_id="1" failure_level="2" inherited="false" overridden="false"/>
 
-  <active_rules id="2" profile_id="2" rule_id="2" failure_level="2" inherited="false" overridden="[null]"/>
+  <active_rules id="2" profile_id="2" rule_id="2" failure_level="2" inherited="false" overridden="false"/>
   
-  <active_rules id="4" profile_id="3" rule_id="2" failure_level="2" inherited="true" overridden="[null]"/>
+  <active_rules id="4" profile_id="3" rule_id="2" failure_level="2" inherited="true" overridden="false"/>
   
 </dataset>
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 @@
   
   <rules_profiles id="3" provided="false" name="child" default_profile="0" language="java" parent_name="parent"/>
 
-  <active_rules id="1" profile_id="1" rule_id="1" failure_level="2" inherited="false" overridden="[null]"/>
+  <active_rules id="1" profile_id="1" rule_id="1" failure_level="2" inherited="false" overridden="false"/>
 
-  <active_rules id="2" profile_id="2" rule_id="2" failure_level="2" inherited="false" overridden="[null]"/>
+  <active_rules id="2" profile_id="2" rule_id="2" failure_level="2" inherited="false" overridden="false"/>
   
-  <active_rules id="3" profile_id="3" rule_id="1" failure_level="2" inherited="true" overridden="[null]"/>
+  <active_rules id="3" profile_id="3" rule_id="1" failure_level="2" inherited="true" overridden="false"/>
   
 </dataset>
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 @@
+<dataset>
+
+  <rules id="1" name="foo" description="test" plugin_config_key="checker/foo"
+         plugin_rule_key="checkstyle.rule1" plugin_name="plugin" enabled="true" cardinality="SINGLE" parent_id="[null]"/>
+
+  <rules_profiles id="1" provided="false" name="level1" default_profile="0" language="java" parent_name="[null]"/>
+  
+  <rules_profiles id="2" provided="false" name="level2" default_profile="0" language="java" parent_name="level1"/>
+  
+  <rules_profiles id="3" provided="false" name="level3" default_profile="0" language="java" parent_name="level2"/>
+
+</dataset>
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 @@
   
   <rules_profiles id="2" provided="false" name="child" default_profile="0" language="java" parent_name="parent"/>
 
-  <active_rules id="1" profile_id="1" rule_id="1" failure_level="2" inherited="false" overridden="[null]"/>
+  <active_rules id="1" profile_id="1" rule_id="1" failure_level="2" inherited="false" overridden="false"/>
 
 </dataset>
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 @@
   
   <rules_profiles id="2" provided="false" name="child" default_profile="0" language="java" parent_name="parent"/>
 
-  <active_rules id="1" profile_id="1" rule_id="1" failure_level="2" inherited="false" overridden="[null]"/>
+  <active_rules id="1" profile_id="1" rule_id="1" failure_level="2" inherited="false" overridden="false"/>
 
-  <active_rules id="2" profile_id="2" rule_id="1" failure_level="2" inherited="true" overridden="[null]"/>
+  <active_rules id="2" profile_id="2" rule_id="1" failure_level="2" inherited="true" overridden="false"/>
 
 </dataset>
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 @@
   
   <rules_profiles id="2" provided="false" name="child" default_profile="0" language="java" parent_name="[null]"/>
 
-  <active_rules id="1" profile_id="1" rule_id="1" failure_level="2" inherited="false" overridden="[null]"/>
+  <active_rules id="1" profile_id="1" rule_id="1" failure_level="2" inherited="false" overridden="false"/>
 
 </dataset>
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 @@
   
   <rules_profiles id="2" provided="false" name="child" default_profile="0" language="java" parent_name="parent"/>
 
-  <active_rules id="1" profile_id="1" rule_id="1" failure_level="2" inherited="false" overridden="[null]"/>
+  <active_rules id="1" profile_id="1" rule_id="1" failure_level="2" inherited="false" overridden="false"/>
 
-  <active_rules id="2" profile_id="2" rule_id="1" failure_level="2" inherited="true" overridden="[null]"/>
+  <active_rules id="2" profile_id="2" rule_id="1" failure_level="2" inherited="true" overridden="false"/>
 
 </dataset>
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 @@
   
   <rules_profiles id="2" provided="false" name="child" default_profile="0" language="java" parent_name="parent"/>
 
-  <active_rules id="1" profile_id="1" rule_id="1" failure_level="2" inherited="false" overridden="[null]"/>
+  <active_rules id="1" profile_id="1" rule_id="1" failure_level="2" inherited="false" overridden="false"/>
 
-  <active_rules id="2" profile_id="2" rule_id="1" failure_level="2" inherited="true" overridden="[null]"/>
+  <active_rules id="2" profile_id="2" rule_id="1" failure_level="2" inherited="true" overridden="false"/>
 
 </dataset>
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 @@
   
   <rules_profiles id="2" provided="false" name="child" default_profile="0" language="java" parent_name="[null]"/>
 
-  <active_rules id="1" profile_id="1" rule_id="1" failure_level="2" inherited="false" overridden="[null]"/>
+  <active_rules id="1" profile_id="1" rule_id="1" failure_level="2" inherited="false" overridden="false"/>
 
 </dataset>