SONAR-4326 Delete unused tags when updating tags on a rule

11 files changed, 127 insertions, 8 deletions

diff --git a/sonar-core/src/main/java/org/sonar/core/rule/RuleTagDao.java b/sonar-core/src/main/java/org/sonar/core/rule/RuleTagDao.java
index 275b73f841b..ea5e88f66a8 100644
--- a/sonar-core/src/main/java/org/sonar/core/rule/RuleTagDao.java
+++ b/sonar-core/src/main/java/org/sonar/core/rule/RuleTagDao.java
@@ -43,7 +43,7 @@ public class RuleTagDao implements ServerExtension {
   }
 
   public List<RuleTagDto> selectAll(SqlSession session) {
-    return session.getMapper(RuleTagMapper.class).selectAll();
+    return getMapper(session).selectAll();
   }
 
   public void insert(RuleTagDto newRuleTag) {
@@ -57,7 +57,7 @@ public class RuleTagDao implements ServerExtension {
   }
 
   public void insert(RuleTagDto newRuleTag, SqlSession session) {
-    session.getMapper(RuleTagMapper.class).insert(newRuleTag);
+    getMapper(session).insert(newRuleTag);
   }
 
   public void delete(Long tagId) {
@@ -71,7 +71,7 @@ public class RuleTagDao implements ServerExtension {
   }
 
   public void delete(long tagId, SqlSession session) {
-    session.getMapper(RuleTagMapper.class).delete(tagId);
+    getMapper(session).delete(tagId);
   }
 
   public Long selectId(String tag) {
@@ -84,6 +84,23 @@ public class RuleTagDao implements ServerExtension {
   }
 
   public Long selectId(String tag, SqlSession session) {
-    return session.getMapper(RuleTagMapper.class).selectId(tag);
+    return getMapper(session).selectId(tag);
+  }
+
+  public List<RuleTagDto> selectUnused() {
+    SqlSession session = myBatis.openSession();
+    try {
+      return selectUnused(session);
+    } finally {
+      MyBatis.closeQuietly(session);
+    }
+  }
+
+  public List<RuleTagDto> selectUnused(SqlSession session) {
+    return getMapper(session).selectUnused();
+  }
+
+  private RuleTagMapper getMapper(SqlSession session) {
+    return session.getMapper(RuleTagMapper.class);
   }
 }
diff --git a/sonar-core/src/main/java/org/sonar/core/rule/RuleTagMapper.java b/sonar-core/src/main/java/org/sonar/core/rule/RuleTagMapper.java
index 5798e0da0da..d3d65c4afe9 100644
--- a/sonar-core/src/main/java/org/sonar/core/rule/RuleTagMapper.java
+++ b/sonar-core/src/main/java/org/sonar/core/rule/RuleTagMapper.java
@@ -30,4 +30,6 @@ public interface RuleTagMapper {
   void delete(Long tagId);
 
   Long selectId(String tag);
+
+  List<RuleTagDto> selectUnused();
 }
diff --git a/sonar-core/src/main/resources/org/sonar/core/rule/RuleTagMapper.xml b/sonar-core/src/main/resources/org/sonar/core/rule/RuleTagMapper.xml
index b2c0f2758f1..5fb61f62a50 100644
--- a/sonar-core/src/main/resources/org/sonar/core/rule/RuleTagMapper.xml
+++ b/sonar-core/src/main/resources/org/sonar/core/rule/RuleTagMapper.xml
@@ -23,5 +23,9 @@
     WHERE tag=#{tag}
   </select>
 
+  <select id="selectUnused" resultType="RuleTag">
+    SELECT r.id, r.tag FROM rule_tags r LEFT JOIN rules_rule_tags rr ON rr.rule_tag_id = r.id WHERE rr.rule_id IS NULL
+  </select>
+
 </mapper>
 
diff --git a/sonar-core/src/test/java/org/sonar/core/rule/RuleTagDaoTest.java b/sonar-core/src/test/java/org/sonar/core/rule/RuleTagDaoTest.java
index a26b1f21639..767dd2e6a9c 100644
--- a/sonar-core/src/test/java/org/sonar/core/rule/RuleTagDaoTest.java
+++ b/sonar-core/src/test/java/org/sonar/core/rule/RuleTagDaoTest.java
@@ -64,4 +64,12 @@ public class RuleTagDaoTest extends AbstractDaoTestCase {
     dao.delete(1L);
     checkTable("delete", "rule_tags");
   }
+
+  @Test
+  public void should_select_unused_tags() {
+    setupData("select-unused");
+
+    assertThat(dao.selectUnused()).hasSize(2);
+  }
+
 }
diff --git a/sonar-core/src/test/resources/org/sonar/core/rule/RuleTagDaoTest/select-unused.xml b/sonar-core/src/test/resources/org/sonar/core/rule/RuleTagDaoTest/select-unused.xml
new file mode 100644
index 00000000000..93bfa7ca339
--- /dev/null
+++ b/sonar-core/src/test/resources/org/sonar/core/rule/RuleTagDaoTest/select-unused.xml
@@ -0,0 +1,15 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<dataset>
+
+  <rules id="1" plugin_rule_key="rule1" plugin_name="fake" plugin_config_key="old_config_key" name="old name" description="old description"
+         status="READY" priority="2" cardinality="SINGLE" parent_id="[null]" />
+
+  <rule_tags id="1" tag="tag1"/>
+  <rule_tags id="2" tag="tag2"/>
+  <rule_tags id="3" tag="tag3"/>
+  <rule_tags id="4" tag="tag4"/>
+
+  <rules_rule_tags id="1" rule_id="1" rule_tag_id="1" tag_type="ADMIN"/>
+  <rules_rule_tags id="2" rule_id="1" rule_tag_id="2" tag_type="SYSTEM"/>
+
+</dataset>
diff --git a/sonar-server/src/main/java/org/sonar/server/qualityprofile/QProfileRuleOperations.java b/sonar-server/src/main/java/org/sonar/server/qualityprofile/QProfileRuleOperations.java
index 213082c4d23..69c5202c4c4 100644
--- a/sonar-server/src/main/java/org/sonar/server/qualityprofile/QProfileRuleOperations.java
+++ b/sonar-server/src/main/java/org/sonar/server/qualityprofile/QProfileRuleOperations.java
@@ -41,6 +41,7 @@ import org.sonar.core.rule.*;
 import org.sonar.server.exceptions.BadRequestException;
 import org.sonar.server.exceptions.NotFoundException;
 import org.sonar.server.rule.RuleRegistry;
+import org.sonar.server.rule.RuleTagOperations;
 import org.sonar.server.user.UserSession;
 
 import java.util.Date;
@@ -56,20 +57,24 @@ public class QProfileRuleOperations implements ServerComponent {
   private final ActiveRuleDao activeRuleDao;
   private final RuleDao ruleDao;
   private final RuleTagDao ruleTagDao;
+  private final RuleTagOperations ruleTagOperations;
   private final RuleRegistry ruleRegistry;
 
   private final System2 system;
 
-  public QProfileRuleOperations(MyBatis myBatis, ActiveRuleDao activeRuleDao, RuleDao ruleDao, RuleTagDao ruleTagDao, RuleRegistry ruleRegistry) {
-    this(myBatis, activeRuleDao, ruleDao, ruleTagDao, ruleRegistry, System2.INSTANCE);
+  public QProfileRuleOperations(MyBatis myBatis, ActiveRuleDao activeRuleDao, RuleDao ruleDao, RuleTagDao ruleTagDao, RuleTagOperations ruleTagOperations,
+    RuleRegistry ruleRegistry) {
+    this(myBatis, activeRuleDao, ruleDao, ruleTagDao, ruleTagOperations, ruleRegistry, System2.INSTANCE);
   }
 
   @VisibleForTesting
-  QProfileRuleOperations(MyBatis myBatis, ActiveRuleDao activeRuleDao, RuleDao ruleDao, RuleTagDao ruleTagDao, RuleRegistry ruleRegistry, System2 system) {
+  QProfileRuleOperations(MyBatis myBatis, ActiveRuleDao activeRuleDao, RuleDao ruleDao, RuleTagDao ruleTagDao, RuleTagOperations ruleTagOperations, RuleRegistry ruleRegistry,
+    System2 system) {
     this.myBatis = myBatis;
     this.activeRuleDao = activeRuleDao;
     this.ruleDao = ruleDao;
     this.ruleTagDao = ruleTagDao;
+    this.ruleTagOperations = ruleTagOperations;
     this.ruleRegistry = ruleRegistry;
     this.system = system;
   }
@@ -282,6 +287,8 @@ public class QProfileRuleOperations implements ServerComponent {
       }
     }
 
+    ruleTagOperations.deleteUnusedTags(session);
+
     return ruleChanged;
   }
 
diff --git a/sonar-server/src/main/java/org/sonar/server/rule/ESRuleTags.java b/sonar-server/src/main/java/org/sonar/server/rule/ESRuleTags.java
index 10d74885453..08bb181bad3 100644
--- a/sonar-server/src/main/java/org/sonar/server/rule/ESRuleTags.java
+++ b/sonar-server/src/main/java/org/sonar/server/rule/ESRuleTags.java
@@ -91,4 +91,8 @@ public class ESRuleTags {
       .field(RuleTagDocument.FIELD_VALUE, tag.getTag())
       .endObject();
   }
+
+  public void delete(String... deleted) {
+    index.bulkDelete(RuleRegistry.INDEX_RULES, TYPE_TAG, deleted);
+  }
 }
diff --git a/sonar-server/src/main/java/org/sonar/server/rule/RuleTagOperations.java b/sonar-server/src/main/java/org/sonar/server/rule/RuleTagOperations.java
index aede30b5d61..6bad6ab7aed 100644
--- a/sonar-server/src/main/java/org/sonar/server/rule/RuleTagOperations.java
+++ b/sonar-server/src/main/java/org/sonar/server/rule/RuleTagOperations.java
@@ -19,6 +19,8 @@
  */
 package org.sonar.server.rule;
 
+import org.apache.ibatis.session.SqlSession;
+import org.elasticsearch.common.collect.Sets;
 import org.sonar.api.ServerExtension;
 import org.sonar.api.server.rule.RuleTagFormat;
 import org.sonar.core.permission.GlobalPermissions;
@@ -27,6 +29,8 @@ import org.sonar.core.rule.RuleTagDto;
 import org.sonar.server.exceptions.BadRequestException;
 import org.sonar.server.user.UserSession;
 
+import java.util.Set;
+
 public class RuleTagOperations implements ServerExtension {
 
   private final RuleTagDao ruleTagDao;
@@ -66,4 +70,15 @@ public class RuleTagOperations implements ServerExtension {
     userSession.checkLoggedIn();
     userSession.checkGlobalPermission(GlobalPermissions.QUALITY_PROFILE_ADMIN);
   }
+
+  public void deleteUnusedTags(SqlSession session) {
+    Set<String> deleted = Sets.newHashSet();
+    for (RuleTagDto unused: ruleTagDao.selectUnused(session)) {
+      deleted.add(unused.getTag());
+      ruleTagDao.delete(unused.getId(), session);
+    }
+    if (!deleted.isEmpty()) {
+      esRuleTags.delete(deleted.toArray(new String[0]));
+    }
+  }
 }
diff --git a/sonar-server/src/test/java/org/sonar/server/qualityprofile/QProfileRuleOperationsTest.java b/sonar-server/src/test/java/org/sonar/server/qualityprofile/QProfileRuleOperationsTest.java
index 6036e2f9655..df2f10868ff 100644
--- a/sonar-server/src/test/java/org/sonar/server/qualityprofile/QProfileRuleOperationsTest.java
+++ b/sonar-server/src/test/java/org/sonar/server/qualityprofile/QProfileRuleOperationsTest.java
@@ -44,6 +44,7 @@ import org.sonar.core.rule.*;
 import org.sonar.server.exceptions.ForbiddenException;
 import org.sonar.server.exceptions.NotFoundException;
 import org.sonar.server.rule.RuleRegistry;
+import org.sonar.server.rule.RuleTagOperations;
 import org.sonar.server.user.MockUserSession;
 import org.sonar.server.user.UserSession;
 
@@ -78,6 +79,9 @@ public class QProfileRuleOperationsTest {
   RuleTagDao ruleTagDao;
 
   @Mock
+  RuleTagOperations ruleTagOperations;
+
+  @Mock
   RuleRegistry ruleRegistry;
 
   @Mock
@@ -104,7 +108,7 @@ public class QProfileRuleOperationsTest {
       }
     }).when(activeRuleDao).insert(any(ActiveRuleDto.class), any(SqlSession.class));
 
-    operations = new QProfileRuleOperations(myBatis, activeRuleDao, ruleDao, ruleTagDao, ruleRegistry, system);
+    operations = new QProfileRuleOperations(myBatis, activeRuleDao, ruleDao, ruleTagDao, ruleTagOperations, ruleRegistry, system);
   }
 
   @Test
@@ -335,6 +339,7 @@ public class QProfileRuleOperationsTest {
     verify(ruleDao, atLeast(1)).selectTags(ruleId, session);
     verify(ruleDao).deleteTag(existingTag, session);
     verify(ruleDao).update(rule, session);
+    verify(ruleTagOperations).deleteUnusedTags(session);
     verify(session).commit();
   }
 
@@ -352,6 +357,7 @@ public class QProfileRuleOperationsTest {
 
     verify(ruleTagDao).selectId(tag, session);
     verify(ruleDao).selectTags(ruleId, session);
+    verify(ruleTagOperations).deleteUnusedTags(session);
     verify(ruleDao, never()).update(rule);
   }
 }
diff --git a/sonar-server/src/test/java/org/sonar/server/rule/ESRuleTagsTest.java b/sonar-server/src/test/java/org/sonar/server/rule/ESRuleTagsTest.java
index 14e223fd954..e4d9486a0dd 100644
--- a/sonar-server/src/test/java/org/sonar/server/rule/ESRuleTagsTest.java
+++ b/sonar-server/src/test/java/org/sonar/server/rule/ESRuleTagsTest.java
@@ -121,6 +121,20 @@ public class ESRuleTagsTest {
     checkTagCount(5L);
   }
 
+  @Test
+  public void should_delete_tags() throws Exception {
+
+    esSetup.execute(
+      indexTagDocument("tag1"),
+      indexTagDocument("tag2"),
+      indexTagDocument("tag3"),
+      indexTagDocument("tag4"));
+
+    ruleTags.delete("tag1", "tag2");
+
+    checkTagCount(2L);
+  }
+
   private void checkTagCount(long count) {
     assertThat(ruleTags.searchAllTags()).hasSize((int) count);
   }
diff --git a/sonar-server/src/test/java/org/sonar/server/rule/RuleTagOperationsTest.java b/sonar-server/src/test/java/org/sonar/server/rule/RuleTagOperationsTest.java
index 99eaf1c50e9..d2fb1e847a9 100644
--- a/sonar-server/src/test/java/org/sonar/server/rule/RuleTagOperationsTest.java
+++ b/sonar-server/src/test/java/org/sonar/server/rule/RuleTagOperationsTest.java
@@ -19,6 +19,8 @@
  */
 package org.sonar.server.rule;
 
+import com.google.common.collect.ImmutableList;
+import org.apache.ibatis.session.SqlSession;
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -34,7 +36,9 @@ import org.sonar.server.user.MockUserSession;
 import org.sonar.server.user.UserSession;
 
 import static org.fest.assertions.Assertions.assertThat;
+import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.verifyNoMoreInteractions;
 import static org.mockito.Mockito.when;
 
 @RunWith(MockitoJUnitRunner.class)
@@ -87,4 +91,27 @@ public class RuleTagOperationsTest {
     assertThat(newTagDto.getValue().getTag()).isEqualTo(newTag);
     verify(esRuleTags).put(newTagDto.getValue());
   }
+
+  @Test
+  public void should_delete_unused_tags() {
+    long tagId = 42L;
+    String tagValue = "answerlifeanevrythng";
+    RuleTagDto unusedTag = new RuleTagDto().setId(tagId).setTag(tagValue);
+    SqlSession session = mock(SqlSession.class);
+    when(ruleTagDao.selectUnused(session)).thenReturn(ImmutableList.of(unusedTag));
+    operations.deleteUnusedTags(session);
+
+    verify(ruleTagDao).selectUnused(session);
+    verify(ruleTagDao).delete(tagId, session);
+    verify(esRuleTags).delete(tagValue);
+  }
+
+  @Test
+  public void should_do_nothing_if_no_unused_tags() {
+    SqlSession session = mock(SqlSession.class);
+    operations.deleteUnusedTags(session);
+
+    verify(ruleTagDao).selectUnused(session);
+    verifyNoMoreInteractions(ruleTagDao, esRuleTags);
+  }
 }