Fix Quality flaws related to org.sonar.server.rule

diff --git a/server/sonar-server/src/main/java/org/sonar/server/rule/RuleService.java b/server/sonar-server/src/main/java/org/sonar/server/rule/RuleService.java
index 581045bf5a1727d895e89c3a76059279bc557eef..48e703743188ea670edd565fe65db662722a6141 100644 (file)
--- a/server/sonar-server/src/main/java/org/sonar/server/rule/RuleService.java
+++ b/server/sonar-server/src/main/java/org/sonar/server/rule/RuleService.java
@@ -77,7 +77,8 @@ public class RuleService {
   }
 
   private void checkPermission() {
-    userSession.checkLoggedIn();
-    userSession.checkPermission(GlobalPermissions.QUALITY_PROFILE_ADMIN);
+    userSession
+      .checkLoggedIn()
+      .checkPermission(GlobalPermissions.QUALITY_PROFILE_ADMIN);
   }
 }

diff --git a/server/sonar-server/src/main/java/org/sonar/server/rule/index/RuleIndex.java b/server/sonar-server/src/main/java/org/sonar/server/rule/index/RuleIndex.java
index d7fe053024f38a6e43e63bab0d6d8af1e19d1a05..626f0ef34993a7dbcc5b0026e7b0c3665d2fa645 100644 (file)
--- a/server/sonar-server/src/main/java/org/sonar/server/rule/index/RuleIndex.java
+++ b/server/sonar-server/src/main/java/org/sonar/server/rule/index/RuleIndex.java
@@ -64,6 +64,7 @@ import org.sonar.server.es.SearchOptions;
 import org.sonar.server.es.StickyFacetBuilder;
 
 import static org.elasticsearch.index.query.QueryBuilders.boolQuery;
+import static org.elasticsearch.index.query.QueryBuilders.matchAllQuery;
 import static org.elasticsearch.index.query.QueryBuilders.matchQuery;
 import static org.elasticsearch.index.query.QueryBuilders.simpleQueryStringQuery;
 import static org.sonar.server.es.EsUtils.SCROLL_TIME_IN_MINUTES;
@@ -168,7 +169,7 @@ public class RuleIndex extends BaseIndex {
     // No contextual query case
     String queryText = query.getQueryText();
     if (StringUtils.isEmpty(queryText)) {
-      return QueryBuilders.matchAllQuery();
+      return matchAllQuery();
     }
 
     // Build RuleBased contextual query
@@ -295,7 +296,7 @@ public class RuleIndex extends BaseIndex {
     if (childrenFilter.hasClauses()) {
       childQuery = childrenFilter;
     } else {
-      childQuery = QueryBuilders.matchAllQuery();
+      childQuery = matchAllQuery();
     }
 
     /** Implementation of activation query */
@@ -469,31 +470,28 @@ public class RuleIndex extends BaseIndex {
   }
 
   public Set<String> terms(String fields, @Nullable String query, int size) {
-    Set<String> tags = new HashSet<>();
-    String key = "_ref";
+    String aggregationKey = "_ref";
 
-    TermsBuilder terms = AggregationBuilders.terms(key)
+    TermsBuilder termsAggregation = AggregationBuilders.terms(aggregationKey)
       .field(fields)
       .size(size)
       .minDocCount(1);
     if (query != null) {
-      terms.include(".*" + query + ".*");
+      termsAggregation.include(".*" + query + ".*");
     }
     SearchRequestBuilder request = getClient()
       .prepareSearch(INDEX)
-      .setQuery(QueryBuilders.matchAllQuery())
-      .addAggregation(terms);
+      .setQuery(matchAllQuery())
+      .addAggregation(termsAggregation);
 
     SearchResponse esResponse = request.get();
 
-    Terms aggregation = esResponse.getAggregations().get(key);
-
+    Set<String> terms = new HashSet<>();
+    Terms aggregation = esResponse.getAggregations().get(aggregationKey);
     if (aggregation != null) {
-      for (Terms.Bucket value : aggregation.getBuckets()) {
-        tags.add(value.getKeyAsString());
-      }
+      aggregation.getBuckets().forEach(value -> terms.add(value.getKeyAsString()));
     }
-    return tags;
+    return terms;
   }
 
   private enum ToRuleKey implements Function<String, RuleKey> {

diff --git a/server/sonar-server/src/test/java/org/sonar/server/rule/RuleServiceMediumTest.java b/server/sonar-server/src/test/java/org/sonar/server/rule/RuleServiceMediumTest.java
index 79f86211988d3fde5b25cedc9a9e54ad8a3a36a8..d56d5aff5b712a8d5b5a07eaae029a303f0614f5 100644 (file)
--- a/server/sonar-server/src/test/java/org/sonar/server/rule/RuleServiceMediumTest.java
+++ b/server/sonar-server/src/test/java/org/sonar/server/rule/RuleServiceMediumTest.java
@@ -19,26 +19,26 @@
  */
 package org.sonar.server.rule;
 
-import com.google.common.collect.Sets;
-import java.util.Collections;
 import java.util.Set;
 import org.junit.After;
 import org.junit.Before;
 import org.junit.ClassRule;
+import org.junit.Rule;
 import org.junit.Test;
+import org.junit.rules.ExpectedException;
 import org.sonar.api.rule.RuleKey;
 import org.sonar.core.permission.GlobalPermissions;
 import org.sonar.db.DbClient;
 import org.sonar.db.DbSession;
 import org.sonar.db.rule.RuleDao;
 import org.sonar.db.rule.RuleTesting;
+import org.sonar.server.exceptions.ForbiddenException;
 import org.sonar.server.exceptions.UnauthorizedException;
-import org.sonar.server.rule.index.RuleIndex;
-import org.sonar.server.rule.index.RuleIndexDefinition;
 import org.sonar.server.rule.index.RuleIndexer;
 import org.sonar.server.tester.ServerTester;
 import org.sonar.server.tester.UserSessionRule;
 
+import static com.google.common.collect.Sets.newHashSet;
 import static org.assertj.core.api.Assertions.assertThat;
 
 public class RuleServiceMediumTest {
@@ -46,14 +46,16 @@ public class RuleServiceMediumTest {
   @ClassRule
   public static ServerTester tester = new ServerTester().withEsIndexes();
 
-  @org.junit.Rule
+  @Rule
   public UserSessionRule userSessionRule = UserSessionRule.forServerTester(tester);
 
-  RuleDao dao = tester.get(RuleDao.class);
-  RuleIndex index = tester.get(RuleIndex.class);
-  RuleService service = tester.get(RuleService.class);
-  DbSession dbSession;
-  RuleIndexer ruleIndexer;
+  @Rule
+  public ExpectedException expectedException = ExpectedException.none();
+
+  private RuleDao dao = tester.get(RuleDao.class);
+  private RuleService service = tester.get(RuleService.class);
+  private DbSession dbSession;
+  private RuleIndexer ruleIndexer;
 
   @Before
   public void before() {
@@ -68,30 +70,53 @@ public class RuleServiceMediumTest {
   }
 
   @Test
-  public void list_tags() {
+  public void listTags_returns_all_tags() {
     // insert db
-    RuleKey key1 = RuleKey.of("javascript", "S001");
-    RuleKey key2 = RuleKey.of("java", "S001");
-    dao.insert(dbSession,
-      RuleTesting.newDto(key1).setTags(Sets.newHashSet("tag1")).setSystemTags(Sets.newHashSet("sys1", "sys2")));
-    dao.insert(dbSession,
-      RuleTesting.newDto(key2).setTags(Sets.newHashSet("tag2")).setSystemTags(Collections.<String>emptySet()));
-    dbSession.commit();
-    ruleIndexer.index();
+    insertRule(RuleKey.of("javascript", "S001"), newHashSet("tag1"), newHashSet("sys1", "sys2"));
+    insertRule(RuleKey.of("java", "S001"), newHashSet("tag2"), newHashSet());
 
     // all tags, including system
     Set<String> tags = service.listTags();
     assertThat(tags).containsOnly("tag1", "tag2", "sys1", "sys2");
+  }
 
-    // verify in es
-    tags = index.terms(RuleIndexDefinition.FIELD_RULE_ALL_TAGS);
-    assertThat(tags).containsOnly("tag1", "tag2", "sys1", "sys2");
+  @Test
+  public void listTags_returns_tags_filtered_by_name() {
+    insertRule(RuleKey.of("javascript", "S001"), newHashSet("tag1"), newHashSet("sys1", "sys2"));
+    insertRule(RuleKey.of("java", "S001"), newHashSet("tag2"), newHashSet());
+
+    assertThat(service.listTags("missing", 10)).isEmpty();
+    assertThat(service.listTags("tag", 10)).containsOnly("tag1", "tag2");
+    assertThat(service.listTags("sys", 10)).containsOnly("sys1", "sys2");
+
+    // LIMITATION: case sensitive
+    assertThat(service.listTags("TAG", 10)).isEmpty();
+    assertThat(service.listTags("TAg", 10)).isEmpty();
+    assertThat(service.listTags("MISSing", 10)).isEmpty();
+  }
+
+  @Test
+  public void delete_throws_UnauthorizedException_if_not_logged_in() {
+    expectedException.expect(UnauthorizedException.class);
+    expectedException.expectMessage("Authentication is required");
+
+    service.delete(RuleKey.of("java", "S001"));
   }
 
-  @Test(expected = UnauthorizedException.class)
-  public void do_not_delete_if_not_granted() {
-    userSessionRule.setGlobalPermissions(GlobalPermissions.SCAN_EXECUTION);
+  @Test
+  public void delete_throws_ForbiddenException_if_not_administrator() {
+    userSessionRule.login().setGlobalPermissions(GlobalPermissions.SCAN_EXECUTION);
+
+    expectedException.expect(ForbiddenException.class);
+    expectedException.expectMessage("Insufficient privileges");
 
     service.delete(RuleKey.of("java", "S001"));
   }
+
+  private void insertRule(RuleKey key, Set<String> tags, Set<String> systemTags) {
+    dao.insert(dbSession,
+      RuleTesting.newDto(key).setTags(tags).setSystemTags(systemTags));
+    dbSession.commit();
+    ruleIndexer.index();
+  }
 }