From 1fd202c62b8f7a45b022720fd6a9968dfff5eda9 Mon Sep 17 00:00:00 2001 From: Jean-Baptiste Lievremont Date: Tue, 3 Mar 2015 13:41:22 +0100 Subject: [PATCH] SONAR-6233 Fix behavior of facet on uncharacterized rules --- .../sonar/server/rule/index/RuleIndex.java | 28 +++++- .../sonar/server/rule/ws/SearchAction.java | 28 ++++++ .../java/org/sonar/server/search/Facets.java | 6 +- .../server/search/StickyFacetBuilder.java | 6 +- .../rule/index/RuleIndexMediumTest.java | 91 +++++++++++++++++++ .../rule/ws/RulesWebServiceMediumTest.java | 35 +++++-- .../search_debt_rule.json | 4 + .../search_debt_rules_sticky.json | 4 + 8 files changed, 188 insertions(+), 14 deletions(-) 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 81df870ca2a..483d6b2c55e 100644 --- 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 @@ -385,24 +385,46 @@ public class RuleIndex extends BaseIndex { } private void addCharacteristicsFacetIfNeeded(RuleQuery query, QueryContext options, Map aggregations, StickyFacetBuilder stickyFacetBuilder) { + if (options.facets().contains(FACET_DEBT_CHARACTERISTICS)) { + /* + * Since this facet concerns 2 fields, we're using an aggregation structure like this: + * global aggregation + * |- sub-aggregation on characteristics: filter + * | |- classic sub-aggregation with top-n terms, excluding NONE + * | |- classic sub-aggregation with selected terms + * | |- terms aggregation on "NONE" + * | |- missing aggregation + * |- sub-aggregation on sub-characteristics: filter, excluding NONE + * |- classic sub-aggregation with top-n terms + * |- classic sub-aggregation with selected terms + */ int characsSize = 10; int subCharacsSize = 300; Collection characsFromQuery = query.getDebtCharacteristics(); Object[] selectedChars = characsFromQuery == null ? new Object[0] : characsFromQuery.toArray(); + BoolFilterBuilder stickyFacetFilter = stickyFacetBuilder.getStickyFacetFilter(FILTER_DEBT_CHARACTERISTICS, FILTER_HAS_DEBT_CHARACTERISTICS); AggregationBuilder debtChar = AggregationBuilders.filter(FACET_DEBT_CHARACTERISTICS + "__chars") - .filter(stickyFacetBuilder.getStickyFacetFilter(FILTER_DEBT_CHARACTERISTICS)) + .filter(stickyFacetFilter) .subAggregation( AggregationBuilders.terms(FACET_DEBT_CHARACTERISTICS + "__chars_top").field(RuleNormalizer.RuleField.CHARACTERISTIC.field()) + .exclude(DebtCharacteristic.NONE) .size(characsSize)) .subAggregation( AggregationBuilders.terms(FACET_DEBT_CHARACTERISTICS + "__chars_selected").field(RuleNormalizer.RuleField.CHARACTERISTIC.field()) .include(Joiner.on('|').join(selectedChars)) - .size(characsSize)); + .size(characsSize)) + .subAggregation( + AggregationBuilders.terms(FACET_DEBT_CHARACTERISTICS + "__chars_none").field(RuleNormalizer.RuleField.CHARACTERISTIC.field()) + .include(DebtCharacteristic.NONE) + .size(characsSize)) + .subAggregation( + AggregationBuilders.missing(FACET_DEBT_CHARACTERISTICS + "__chars_missing").field(RuleNormalizer.RuleField.CHARACTERISTIC.field())); AggregationBuilder debtSubChar = AggregationBuilders.filter(FACET_DEBT_CHARACTERISTICS + "__subchars") - .filter(stickyFacetBuilder.getStickyFacetFilter(FILTER_DEBT_CHARACTERISTICS)) + .filter(stickyFacetFilter) .subAggregation( AggregationBuilders.terms(FACET_DEBT_CHARACTERISTICS + "__subchars_top").field(RuleNormalizer.RuleField.SUB_CHARACTERISTIC.field()) + .exclude(DebtCharacteristic.NONE) .size(subCharacsSize)) .subAggregation( AggregationBuilders.terms(FACET_DEBT_CHARACTERISTICS + "__chars_selected").field(RuleNormalizer.RuleField.SUB_CHARACTERISTIC.field()) diff --git a/server/sonar-server/src/main/java/org/sonar/server/rule/ws/SearchAction.java b/server/sonar-server/src/main/java/org/sonar/server/rule/ws/SearchAction.java index d2c00afe29b..66bbea4f747 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/rule/ws/SearchAction.java +++ b/server/sonar-server/src/main/java/org/sonar/server/rule/ws/SearchAction.java @@ -25,6 +25,7 @@ import com.google.common.collect.ImmutableSet; import com.google.common.io.Resources; import org.sonar.api.rule.RuleStatus; import org.sonar.api.rule.Severity; +import org.sonar.api.server.debt.DebtCharacteristic; import org.sonar.api.server.ws.Request; import org.sonar.api.server.ws.WebService; import org.sonar.api.utils.text.JsonWriter; @@ -35,6 +36,7 @@ import org.sonar.server.rule.RuleService; import org.sonar.server.rule.index.RuleIndex; import org.sonar.server.rule.index.RuleNormalizer; import org.sonar.server.rule.index.RuleQuery; +import org.sonar.server.search.FacetValue; import org.sonar.server.search.QueryContext; import org.sonar.server.search.Result; import org.sonar.server.search.ws.SearchOptions; @@ -44,6 +46,7 @@ import javax.annotation.CheckForNull; import java.util.Arrays; import java.util.Collection; +import java.util.Iterator; /** * @since 4.4 @@ -314,6 +317,31 @@ public class SearchAction extends SearchRequestHandler implemen addMandatoryFacetValues(results, RuleIndex.FACET_STATUSES, RuleIndex.ALL_STATUSES_EXCEPT_REMOVED); addMandatoryFacetValues(results, RuleIndex.FACET_SEVERITIES, Severity.ALL); addMandatoryFacetValues(results, RuleIndex.FACET_TAGS, request.paramAsStrings(PARAM_TAGS)); + + mergeNoneAndEmptyBucketOnCharacteristics(results); + super.writeFacets(request, context, results, json); } + + protected void mergeNoneAndEmptyBucketOnCharacteristics(Result results) { + if (results.getFacets().containsKey(RuleIndex.FACET_DEBT_CHARACTERISTICS)) { + Collection characValues = results.getFacetValues(RuleIndex.FACET_DEBT_CHARACTERISTICS); + if (characValues == null) { + return; + } + + long mergedCount = 0L; + Iterator characIterator = characValues.iterator(); + while (characIterator.hasNext()) { + FacetValue characValue = characIterator.next(); + if ("".equals(characValue.getKey()) || DebtCharacteristic.NONE.equals(characValue.getKey())) { + mergedCount += characValue.getValue(); + characIterator.remove(); + } + } + + FacetValue mergedNoneValue = new FacetValue(DebtCharacteristic.NONE, mergedCount); + characValues.add(mergedNoneValue); + } + } } diff --git a/server/sonar-server/src/main/java/org/sonar/server/search/Facets.java b/server/sonar-server/src/main/java/org/sonar/server/search/Facets.java index 58f6b140bc4..2b23c003331 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/search/Facets.java +++ b/server/sonar-server/src/main/java/org/sonar/server/search/Facets.java @@ -71,7 +71,11 @@ class Facets { Missing missing = (Missing) aggregation; long docCount = missing.getDocCount(); if (docCount > 0L) { - this.facetValues.put(aggregation.getName().replace("_missing", ""), new FacetValue("", docCount)); + String facetName = aggregation.getName(); + if (facetName.contains("__") && !facetName.startsWith("__")) { + facetName = facetName.substring(0, facetName.indexOf("__")); + } + this.facetValues.put(facetName.replace("_missing", ""), new FacetValue("", docCount)); } } diff --git a/server/sonar-server/src/main/java/org/sonar/server/search/StickyFacetBuilder.java b/server/sonar-server/src/main/java/org/sonar/server/search/StickyFacetBuilder.java index 53b36c093ba..589536f1484 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/search/StickyFacetBuilder.java +++ b/server/sonar-server/src/main/java/org/sonar/server/search/StickyFacetBuilder.java @@ -20,7 +20,7 @@ package org.sonar.server.search; import com.google.common.base.Joiner; -import org.apache.commons.lang.StringUtils; +import org.apache.commons.lang.ArrayUtils; import org.elasticsearch.index.query.BoolFilterBuilder; import org.elasticsearch.index.query.FilterBuilder; import org.elasticsearch.index.query.FilterBuilders; @@ -67,10 +67,10 @@ public class StickyFacetBuilder { .subAggregation(facetTopAggregation); } - public BoolFilterBuilder getStickyFacetFilter(String fieldName) { + public BoolFilterBuilder getStickyFacetFilter(String... fieldNames) { BoolFilterBuilder facetFilter = FilterBuilders.boolFilter().must(FilterBuilders.queryFilter(query)); for (Map.Entry filter : filters.entrySet()) { - if (filter.getValue() != null && !StringUtils.equals(filter.getKey(), fieldName)) { + if (filter.getValue() != null && !ArrayUtils.contains(fieldNames, filter.getKey())) { facetFilter.must(filter.getValue()); } } diff --git a/server/sonar-server/src/test/java/org/sonar/server/rule/index/RuleIndexMediumTest.java b/server/sonar-server/src/test/java/org/sonar/server/rule/index/RuleIndexMediumTest.java index 97b36033208..10f7dfba5da 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/rule/index/RuleIndexMediumTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/rule/index/RuleIndexMediumTest.java @@ -340,6 +340,97 @@ public class RuleIndexMediumTest { assertThat(index.search(new RuleQuery().setHasDebtCharacteristic(false), new QueryContext()).getTotal()).isEqualTo(2); } + @Test + public void facet_on_debt_characteristic() throws InterruptedException { + CharacteristicDto char1 = DebtTesting.newCharacteristicDto("c1") + .setEnabled(true) + .setName("char1"); + db.debtCharacteristicDao().insert(char1, dbSession); + dbSession.commit(); + + CharacteristicDto char11 = DebtTesting.newCharacteristicDto("c11") + .setEnabled(true) + .setName("char11") + .setParentId(char1.getId()); + db.debtCharacteristicDao().insert(char11, dbSession); + + // Rule with default characteristic + dao.insert(dbSession, RuleTesting.newDto(RuleKey.of("findbugs", "S001")) + .setSubCharacteristicId(null) + .setRemediationFunction(null) + .setDefaultSubCharacteristicId(char11.getId()) + .setDefaultRemediationFunction("LINEAR").setDefaultRemediationCoefficient("2h")); + // Rule with overridden characteristic + dao.insert(dbSession, RuleTesting.newDto(RuleKey.of("pmd", "S002")) + .setSubCharacteristicId(char11.getId()) + .setRemediationFunction("LINEAR").setRemediationCoefficient("2h") + .setDefaultSubCharacteristicId(null) + .setDefaultRemediationFunction(null)); + // Rule without debt characteristic + dao.insert(dbSession, RuleTesting.newDto(RuleKey.of("xoo", "S001")) + .setSubCharacteristicId(null) + .setRemediationFunction(null).setRemediationCoefficient(null) + .setDefaultSubCharacteristicId(null) + .setDefaultRemediationFunction(null).setDefaultRemediationCoefficient(null)); + // Rule with disabled debt characteristic + dao.insert(dbSession, RuleTesting.newDto(RuleKey.of("xoo", "S002")) + .setSubCharacteristicId(-1) + .setRemediationFunction(null).setRemediationCoefficient(null) + .setDefaultSubCharacteristicId(null) + .setDefaultRemediationFunction(null).setDefaultRemediationCoefficient(null)); + dbSession.commit(); + + QueryContext withDebtCharFacet = new QueryContext().addFacets(Arrays.asList(RuleIndex.FACET_DEBT_CHARACTERISTICS)); + + // Facet show results on characs, subcharacs and uncharacterized rules + Result result1 = index.search(new RuleQuery(), withDebtCharFacet); + assertThat(result1.getFacetValues(RuleIndex.FACET_DEBT_CHARACTERISTICS)).containsOnly( + new FacetValue("c1", 2L), + new FacetValue("c11", 2L), + new FacetValue("", 1L), + new FacetValue("NONE", 1L) + ); + + // Facet is sticky when using a charac filter + Result result2 = index.search(new RuleQuery().setDebtCharacteristics(Arrays.asList("c1")), withDebtCharFacet); + assertThat(result2.getFacetValues(RuleIndex.FACET_DEBT_CHARACTERISTICS)).containsOnly( + new FacetValue("c1", 2L), + new FacetValue("c11", 2L), + new FacetValue("", 1L), + new FacetValue("NONE", 1L) + ); + + // Facet is sticky when using a sub-charac filter + Result result3 = index.search(new RuleQuery().setDebtCharacteristics(Arrays.asList("c11")), withDebtCharFacet); + assertThat(result3.getFacetValues(RuleIndex.FACET_DEBT_CHARACTERISTICS)).containsOnly( + new FacetValue("c1", 2L), + new FacetValue("c11", 2L), + new FacetValue("", 1L), + new FacetValue("NONE", 1L) + ); + + // Facet is sticky when using hasCharac filter + Result result4 = index.search(new RuleQuery().setHasDebtCharacteristic(false), withDebtCharFacet); + assertThat(result4.getFacetValues(RuleIndex.FACET_DEBT_CHARACTERISTICS)).containsOnly( + new FacetValue("c1", 2L), + new FacetValue("c11", 2L), + new FacetValue("", 1L), + new FacetValue("NONE", 1L) + ); + + // Facet applies other filters + Result result5 = index.search(new RuleQuery().setRepositories(Arrays.asList("xoo")), withDebtCharFacet); + assertThat(result5.getFacetValues(RuleIndex.FACET_DEBT_CHARACTERISTICS)).containsOnly( + new FacetValue("", 1L), + new FacetValue("NONE", 1L) + ); + Result result6 = index.search(new RuleQuery().setRepositories(Arrays.asList("findbugs")), withDebtCharFacet); + assertThat(result6.getFacetValues(RuleIndex.FACET_DEBT_CHARACTERISTICS)).containsOnly( + new FacetValue("c1", 1L), + new FacetValue("c11", 1L) + ); + } + @Test public void search_by_any_of_repositories() { dao.insert(dbSession, RuleTesting.newDto(RuleKey.of("findbugs", "S001"))); diff --git a/server/sonar-server/src/test/java/org/sonar/server/rule/ws/RulesWebServiceMediumTest.java b/server/sonar-server/src/test/java/org/sonar/server/rule/ws/RulesWebServiceMediumTest.java index eca79f58f1c..4a97bf50e68 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/rule/ws/RulesWebServiceMediumTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/rule/ws/RulesWebServiceMediumTest.java @@ -19,8 +19,6 @@ */ package org.sonar.server.rule.ws; -import org.sonar.core.rule.RuleDto.Format; - import com.google.common.collect.ImmutableSet; import org.junit.After; import org.junit.Before; @@ -38,6 +36,7 @@ import org.sonar.core.qualityprofile.db.ActiveRuleParamDto; import org.sonar.core.qualityprofile.db.QualityProfileDao; import org.sonar.core.qualityprofile.db.QualityProfileDto; import org.sonar.core.rule.RuleDto; +import org.sonar.core.rule.RuleDto.Format; import org.sonar.core.rule.RuleParamDto; import org.sonar.core.technicaldebt.db.CharacteristicDto; import org.sonar.server.db.DbClient; @@ -316,14 +315,36 @@ public class RulesWebServiceMediumTest { .setRemediationCoefficient("30min") .setRemediationOffset("5min") ); + ruleDao.insert(session, RuleTesting.newXooX3() + .setDefaultSubCharacteristicId(null) + .setDefaultRemediationFunction(DebtRemediationFunction.Type.LINEAR_OFFSET.name()) + .setDefaultRemediationCoefficient("2min") + .setDefaultRemediationOffset("1min") + + .setSubCharacteristicId(null) + .setRemediationFunction(null) + .setRemediationCoefficient(null) + .setRemediationOffset(null) + ); + ruleDao.insert(session, RuleTesting.newDto(RuleKey.of("xoo", "x4")).setLanguage("xoo") + .setDefaultSubCharacteristicId(softReliabilityId) + .setDefaultRemediationFunction(DebtRemediationFunction.Type.LINEAR_OFFSET.name()) + .setDefaultRemediationCoefficient("2min") + .setDefaultRemediationOffset("1min") + + .setSubCharacteristicId(-1) + .setRemediationFunction(null) + .setRemediationCoefficient(null) + .setRemediationOffset(null) + ); session.commit(); MockUserSession.set(); - WsTester.TestRequest request = tester.wsTester().newGetRequest(API_ENDPOINT, API_SEARCH_METHOD); - request.setParam(SearchOptions.PARAM_FIELDS, "debtChar,debtCharName,debtSubChar,debtSubCharName,debtRemFn,debtOverloaded,defaultDebtChar,defaultDebtSubChar,defaultDebtRemFn"); - request.setParam("debt_characteristics", "SOFT_RELIABILITY"); - request.setParam(SearchAction.PARAM_FACETS, "debt_characteristics"); - WsTester.Result result = request.execute(); + WsTester.Result result = tester.wsTester().newGetRequest(API_ENDPOINT, API_SEARCH_METHOD) + .setParam(SearchOptions.PARAM_FIELDS, "debtChar,debtCharName,debtSubChar,debtSubCharName,debtRemFn,debtOverloaded,defaultDebtChar,defaultDebtSubChar,defaultDebtRemFn") + .setParam("debt_characteristics", "SOFT_RELIABILITY") + .setParam(SearchAction.PARAM_FACETS, "debt_characteristics") + .execute(); result.assertJson(this.getClass(), "search_debt_rules_sticky.json", false); } diff --git a/server/sonar-server/src/test/resources/org/sonar/server/rule/ws/RulesWebServiceMediumTest/search_debt_rule.json b/server/sonar-server/src/test/resources/org/sonar/server/rule/ws/RulesWebServiceMediumTest/search_debt_rule.json index cd218765544..0a2211290ec 100644 --- a/server/sonar-server/src/test/resources/org/sonar/server/rule/ws/RulesWebServiceMediumTest/search_debt_rule.json +++ b/server/sonar-server/src/test/resources/org/sonar/server/rule/ws/RulesWebServiceMediumTest/search_debt_rule.json @@ -27,6 +27,10 @@ { "val": "HARD_RELIABILITY", "count": 1 + }, + { + "val": "NONE", + "count": 0 } ] } diff --git a/server/sonar-server/src/test/resources/org/sonar/server/rule/ws/RulesWebServiceMediumTest/search_debt_rules_sticky.json b/server/sonar-server/src/test/resources/org/sonar/server/rule/ws/RulesWebServiceMediumTest/search_debt_rules_sticky.json index fdd626cfb0e..a7cba30b88e 100644 --- a/server/sonar-server/src/test/resources/org/sonar/server/rule/ws/RulesWebServiceMediumTest/search_debt_rules_sticky.json +++ b/server/sonar-server/src/test/resources/org/sonar/server/rule/ws/RulesWebServiceMediumTest/search_debt_rules_sticky.json @@ -31,6 +31,10 @@ { "val": "HARD_RELIABILITY", "count": 1 + }, + { + "val": "NONE", + "count": 2 } ] } -- 2.39.5