From 042ce5affc29bf7bd735aa9314cd2f27f86ab4ff Mon Sep 17 00:00:00 2001 From: Jean-Baptiste Lievremont Date: Tue, 14 Apr 2015 14:51:52 +0200 Subject: [PATCH] SONAR-6303 Apply feedback from PR --- .../server/platform/ServerComponents.java | 2 +- ...omparator.java => QProfileComparison.java} | 14 +- .../ws/QProfileCompareAction.java | 10 +- .../QProfileComparisonMediumTest.java | 242 ++++++++++++++++++ .../ws/QProfileCompareActionMediumTest.java | 36 +++ .../compare_param_on_left.json | 32 +++ .../compare_param_on_right.json | 32 +++ 7 files changed, 352 insertions(+), 16 deletions(-) rename server/sonar-server/src/main/java/org/sonar/server/qualityprofile/{QProfileComparator.java => QProfileComparison.java} (92%) create mode 100644 server/sonar-server/src/test/java/org/sonar/server/qualityprofile/QProfileComparisonMediumTest.java create mode 100644 server/sonar-server/src/test/resources/org/sonar/server/qualityprofile/ws/QProfileCompareActionMediumTest/compare_param_on_left.json create mode 100644 server/sonar-server/src/test/resources/org/sonar/server/qualityprofile/ws/QProfileCompareActionMediumTest/compare_param_on_right.json diff --git a/server/sonar-server/src/main/java/org/sonar/server/platform/ServerComponents.java b/server/sonar-server/src/main/java/org/sonar/server/platform/ServerComponents.java index 95bad79553e..70c8a708587 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/platform/ServerComponents.java +++ b/server/sonar-server/src/main/java/org/sonar/server/platform/ServerComponents.java @@ -377,7 +377,7 @@ class ServerComponents { pico.addSingleton(QProfileLookup.class); pico.addSingleton(QProfileProjectOperations.class); pico.addSingleton(QProfileProjectLookup.class); - pico.addSingleton(QProfileComparator.class); + pico.addSingleton(QProfileComparison.class); pico.addSingleton(BuiltInProfiles.class); pico.addSingleton(QProfileRestoreBuiltInAction.class); pico.addSingleton(QProfileSearchAction.class); diff --git a/server/sonar-server/src/main/java/org/sonar/server/qualityprofile/QProfileComparator.java b/server/sonar-server/src/main/java/org/sonar/server/qualityprofile/QProfileComparison.java similarity index 92% rename from server/sonar-server/src/main/java/org/sonar/server/qualityprofile/QProfileComparator.java rename to server/sonar-server/src/main/java/org/sonar/server/qualityprofile/QProfileComparison.java index 767dfaa5c17..b37bcc0eef7 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/qualityprofile/QProfileComparator.java +++ b/server/sonar-server/src/main/java/org/sonar/server/qualityprofile/QProfileComparison.java @@ -34,13 +34,13 @@ import java.util.Collection; import java.util.Map; import java.util.Set; -public class QProfileComparator implements ServerComponent { +public class QProfileComparison implements ServerComponent { private final DbClient dbClient; private final QProfileLoader profileLoader; - public QProfileComparator(DbClient dbClient, QProfileLoader profileLoader) { + public QProfileComparison(DbClient dbClient, QProfileLoader profileLoader) { this.dbClient = dbClient; this.profileLoader = profileLoader; } @@ -78,18 +78,17 @@ public class QProfileComparator implements ServerComponent { } else if (!rightActiveRulesByRuleKey.containsKey(ruleKey)) { result.inLeft.put(ruleKey, leftActiveRulesByRuleKey.get(ruleKey)); } else { - compare(leftActiveRulesByRuleKey.get(ruleKey), rightActiveRulesByRuleKey.get(ruleKey), result); + compareActivationParams(leftActiveRulesByRuleKey.get(ruleKey), rightActiveRulesByRuleKey.get(ruleKey), result); } } } - private void compare(ActiveRule leftRule, ActiveRule rightRule, QProfileComparisonResult result) { + private void compareActivationParams(ActiveRule leftRule, ActiveRule rightRule, QProfileComparisonResult result) { RuleKey key = leftRule.key().ruleKey(); if (leftRule.params().equals(rightRule.params()) && leftRule.severity().equals(rightRule.severity())) { result.same.put(key, leftRule); } else { ActiveRuleDiff diff = new ActiveRuleDiff(); - diff.key = key; if (leftRule.severity().equals(rightRule.severity())) { diff.leftSeverity = diff.rightSeverity = leftRule.severity(); @@ -156,15 +155,10 @@ public class QProfileComparator implements ServerComponent { } public static class ActiveRuleDiff { - private RuleKey key; private String leftSeverity; private String rightSeverity; private MapDifference paramDifference; - public RuleKey key() { - return key; - } - public String leftSeverity() { return leftSeverity; } diff --git a/server/sonar-server/src/main/java/org/sonar/server/qualityprofile/ws/QProfileCompareAction.java b/server/sonar-server/src/main/java/org/sonar/server/qualityprofile/ws/QProfileCompareAction.java index 754095e8512..44b72f3bcff 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/qualityprofile/ws/QProfileCompareAction.java +++ b/server/sonar-server/src/main/java/org/sonar/server/qualityprofile/ws/QProfileCompareAction.java @@ -32,9 +32,9 @@ import org.sonar.api.utils.text.JsonWriter; import org.sonar.core.qualityprofile.db.QualityProfileDto; import org.sonar.core.util.NonNullInputFunction; import org.sonar.server.qualityprofile.ActiveRule; -import org.sonar.server.qualityprofile.QProfileComparator; -import org.sonar.server.qualityprofile.QProfileComparator.ActiveRuleDiff; -import org.sonar.server.qualityprofile.QProfileComparator.QProfileComparisonResult; +import org.sonar.server.qualityprofile.QProfileComparison; +import org.sonar.server.qualityprofile.QProfileComparison.ActiveRuleDiff; +import org.sonar.server.qualityprofile.QProfileComparison.QProfileComparisonResult; import org.sonar.server.rule.Rule; import org.sonar.server.rule.RuleRepositories; import org.sonar.server.rule.RuleRepositories.Repository; @@ -49,12 +49,12 @@ public class QProfileCompareAction implements BaseQProfileWsAction { private static final String PARAM_LEFT_KEY = "leftKey"; private static final String PARAM_RIGHT_KEY = "rightKey"; - private final QProfileComparator comparator; + private final QProfileComparison comparator; private final RuleService ruleService; private final RuleRepositories ruleRepositories; private final Languages languages; - public QProfileCompareAction(QProfileComparator comparator, RuleService ruleService, RuleRepositories ruleRepositories, Languages languages) { + public QProfileCompareAction(QProfileComparison comparator, RuleService ruleService, RuleRepositories ruleRepositories, Languages languages) { this.comparator = comparator; this.ruleService = ruleService; this.ruleRepositories = ruleRepositories; diff --git a/server/sonar-server/src/test/java/org/sonar/server/qualityprofile/QProfileComparisonMediumTest.java b/server/sonar-server/src/test/java/org/sonar/server/qualityprofile/QProfileComparisonMediumTest.java new file mode 100644 index 00000000000..2034d966e70 --- /dev/null +++ b/server/sonar-server/src/test/java/org/sonar/server/qualityprofile/QProfileComparisonMediumTest.java @@ -0,0 +1,242 @@ +/* + * SonarQube, open source software quality management tool. + * Copyright (C) 2008-2014 SonarSource + * mailto:contact AT sonarsource DOT com + * + * SonarQube is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 3 of the License, or (at your option) any later version. + * + * SonarQube is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this program; if not, write to the Free Software Foundation, + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + */ +package org.sonar.server.qualityprofile; + +import com.google.common.collect.MapDifference.ValueDifference; +import org.assertj.core.data.MapEntry; +import org.junit.After; +import org.junit.Before; +import org.junit.ClassRule; +import org.junit.Test; +import org.sonar.api.rule.Severity; +import org.sonar.api.server.rule.RuleParamType; +import org.sonar.core.persistence.DbSession; +import org.sonar.core.qualityprofile.db.QualityProfileDto; +import org.sonar.core.rule.RuleDto; +import org.sonar.core.rule.RuleParamDto; +import org.sonar.server.db.DbClient; +import org.sonar.server.qualityprofile.QProfileComparison.ActiveRuleDiff; +import org.sonar.server.qualityprofile.QProfileComparison.QProfileComparisonResult; +import org.sonar.server.qualityprofile.index.ActiveRuleIndex; +import org.sonar.server.rule.RuleTesting; +import org.sonar.server.tester.ServerTester; + +import static org.assertj.core.api.Assertions.assertThat; + +public class QProfileComparisonMediumTest { + + @ClassRule + public static ServerTester tester = new ServerTester(); + + DbClient db; + DbSession dbSession; + ActiveRuleIndex index; + RuleActivator ruleActivator; + QProfileComparison comparison; + + RuleDto xooRule1; + RuleDto xooRule2; + QualityProfileDto left; + QualityProfileDto right; + + @Before + public void before() { + tester.clearDbAndIndexes(); + db = tester.get(DbClient.class); + dbSession = db.openSession(false); + ruleActivator = tester.get(RuleActivator.class); + comparison = tester.get(QProfileComparison.class); + + xooRule1 = RuleTesting.newXooX1().setSeverity("MINOR"); + xooRule2 = RuleTesting.newXooX2().setSeverity("MAJOR"); + db.ruleDao().insert(dbSession, xooRule1, xooRule2); + db.ruleDao().addRuleParam(dbSession, xooRule1, RuleParamDto.createFor(xooRule1) + .setName("max").setType(RuleParamType.INTEGER.type())); + db.ruleDao().addRuleParam(dbSession, xooRule1, RuleParamDto.createFor(xooRule1) + .setName("min").setType(RuleParamType.INTEGER.type())); + + left = QProfileTesting.newXooP1(); + right = QProfileTesting.newXooP2(); + db.qualityProfileDao().insert(dbSession, left, right); + dbSession.commit(); + dbSession.clearCache(); + } + + @After + public void after() throws Exception { + dbSession.close(); + } + + @Test + public void compare_empty_profiles() throws Exception { + QProfileComparisonResult result = comparison.compare(left.getKey(), right.getKey()); + assertThat(result.left().getKey()).isEqualTo(left.getKey()); + assertThat(result.right().getKey()).isEqualTo(right.getKey()); + assertThat(result.same()).isEmpty(); + assertThat(result.inLeft()).isEmpty(); + assertThat(result.inRight()).isEmpty(); + assertThat(result.modified()).isEmpty(); + assertThat(result.collectRuleKeys()).isEmpty(); + } + + @Test + public void compare_same() throws Exception { + RuleActivation commonActivation = new RuleActivation(xooRule1.getKey()) + .setSeverity(Severity.CRITICAL) + .setParameter("min", "7") + .setParameter("max", "42"); + ruleActivator.activate(dbSession, commonActivation, left); + ruleActivator.activate(dbSession, commonActivation, right); + dbSession.commit(); + + QProfileComparisonResult result = comparison.compare(left.getKey(), right.getKey()); + assertThat(result.left().getKey()).isEqualTo(left.getKey()); + assertThat(result.right().getKey()).isEqualTo(right.getKey()); + assertThat(result.same()).isNotEmpty().containsOnlyKeys(xooRule1.getKey()); + assertThat(result.inLeft()).isEmpty(); + assertThat(result.inRight()).isEmpty(); + assertThat(result.modified()).isEmpty(); + assertThat(result.collectRuleKeys()).containsOnly(xooRule1.getKey()); + } + + @Test + public void compare_only_left() throws Exception { + ruleActivator.activate(dbSession, new RuleActivation(xooRule1.getKey()), left); + dbSession.commit(); + + QProfileComparisonResult result = comparison.compare(left.getKey(), right.getKey()); + assertThat(result.left().getKey()).isEqualTo(left.getKey()); + assertThat(result.right().getKey()).isEqualTo(right.getKey()); + assertThat(result.same()).isEmpty(); + assertThat(result.inLeft()).isNotEmpty().containsOnlyKeys(xooRule1.getKey()); + assertThat(result.inRight()).isEmpty(); + assertThat(result.modified()).isEmpty(); + assertThat(result.collectRuleKeys()).containsOnly(xooRule1.getKey()); + } + + @Test + public void compare_only_right() throws Exception { + ruleActivator.activate(dbSession, new RuleActivation(xooRule1.getKey()), right); + dbSession.commit(); + + QProfileComparisonResult result = comparison.compare(left.getKey(), right.getKey()); + assertThat(result.left().getKey()).isEqualTo(left.getKey()); + assertThat(result.right().getKey()).isEqualTo(right.getKey()); + assertThat(result.same()).isEmpty(); + assertThat(result.inLeft()).isEmpty(); + assertThat(result.inRight()).isNotEmpty().containsOnlyKeys(xooRule1.getKey()); + assertThat(result.modified()).isEmpty(); + assertThat(result.collectRuleKeys()).containsOnly(xooRule1.getKey()); + } + + @Test + public void compare_disjoint() throws Exception { + ruleActivator.activate(dbSession, new RuleActivation(xooRule1.getKey()), left); + ruleActivator.activate(dbSession, new RuleActivation(xooRule2.getKey()), right); + dbSession.commit(); + + QProfileComparisonResult result = comparison.compare(left.getKey(), right.getKey()); + assertThat(result.left().getKey()).isEqualTo(left.getKey()); + assertThat(result.right().getKey()).isEqualTo(right.getKey()); + assertThat(result.same()).isEmpty(); + assertThat(result.inLeft()).isNotEmpty().containsOnlyKeys(xooRule1.getKey()); + assertThat(result.inRight()).isNotEmpty().containsOnlyKeys(xooRule2.getKey()); + assertThat(result.modified()).isEmpty(); + assertThat(result.collectRuleKeys()).containsOnly(xooRule1.getKey(), xooRule2.getKey()); + } + + @Test + public void compare_modified_severity() throws Exception { + ruleActivator.activate(dbSession, new RuleActivation(xooRule1.getKey()).setSeverity(Severity.CRITICAL), left); + ruleActivator.activate(dbSession, new RuleActivation(xooRule1.getKey()).setSeverity(Severity.BLOCKER), right); + dbSession.commit(); + + QProfileComparisonResult result = comparison.compare(left.getKey(), right.getKey()); + assertThat(result.left().getKey()).isEqualTo(left.getKey()); + assertThat(result.right().getKey()).isEqualTo(right.getKey()); + assertThat(result.same()).isEmpty(); + assertThat(result.inLeft()).isEmpty(); + assertThat(result.inRight()).isEmpty(); + assertThat(result.modified()).isNotEmpty().containsOnlyKeys(xooRule1.getKey()); + assertThat(result.collectRuleKeys()).containsOnly(xooRule1.getKey()); + + ActiveRuleDiff activeRuleDiff = result.modified().get(xooRule1.getKey()); + assertThat(activeRuleDiff.leftSeverity()).isEqualTo(Severity.CRITICAL); + assertThat(activeRuleDiff.rightSeverity()).isEqualTo(Severity.BLOCKER); + assertThat(activeRuleDiff.paramDifference().areEqual()).isTrue(); + } + + @Test + public void compare_modified_param() throws Exception { + ruleActivator.activate(dbSession, new RuleActivation(xooRule1.getKey()).setParameter("max", "20"), left); + ruleActivator.activate(dbSession, new RuleActivation(xooRule1.getKey()).setParameter("max", "30"), right); + dbSession.commit(); + + QProfileComparisonResult result = comparison.compare(left.getKey(), right.getKey()); + assertThat(result.left().getKey()).isEqualTo(left.getKey()); + assertThat(result.right().getKey()).isEqualTo(right.getKey()); + assertThat(result.same()).isEmpty(); + assertThat(result.inLeft()).isEmpty(); + assertThat(result.inRight()).isEmpty(); + assertThat(result.modified()).isNotEmpty().containsOnlyKeys(xooRule1.getKey()); + assertThat(result.collectRuleKeys()).containsOnly(xooRule1.getKey()); + + ActiveRuleDiff activeRuleDiff = result.modified().get(xooRule1.getKey()); + assertThat(activeRuleDiff.leftSeverity()).isEqualTo(activeRuleDiff.rightSeverity()).isEqualTo(xooRule1.getSeverityString()); + assertThat(activeRuleDiff.paramDifference().areEqual()).isFalse(); + assertThat(activeRuleDiff.paramDifference().entriesDiffering()).isNotEmpty(); + ValueDifference paramDiff = activeRuleDiff.paramDifference().entriesDiffering().get("max"); + assertThat(paramDiff.leftValue()).isEqualTo("20"); + assertThat(paramDiff.rightValue()).isEqualTo("30"); + } + + @Test + public void compare_different_params() throws Exception { + ruleActivator.activate(dbSession, new RuleActivation(xooRule1.getKey()).setParameter("max", "20"), left); + ruleActivator.activate(dbSession, new RuleActivation(xooRule1.getKey()).setParameter("min", "5"), right); + dbSession.commit(); + + QProfileComparisonResult result = comparison.compare(left.getKey(), right.getKey()); + assertThat(result.left().getKey()).isEqualTo(left.getKey()); + assertThat(result.right().getKey()).isEqualTo(right.getKey()); + assertThat(result.same()).isEmpty(); + assertThat(result.inLeft()).isEmpty(); + assertThat(result.inRight()).isEmpty(); + assertThat(result.modified()).isNotEmpty().containsOnlyKeys(xooRule1.getKey()); + assertThat(result.collectRuleKeys()).containsOnly(xooRule1.getKey()); + + ActiveRuleDiff activeRuleDiff = result.modified().get(xooRule1.getKey()); + assertThat(activeRuleDiff.leftSeverity()).isEqualTo(activeRuleDiff.rightSeverity()).isEqualTo(xooRule1.getSeverityString()); + assertThat(activeRuleDiff.paramDifference().areEqual()).isFalse(); + assertThat(activeRuleDiff.paramDifference().entriesDiffering()).isEmpty(); + assertThat(activeRuleDiff.paramDifference().entriesOnlyOnLeft()).containsExactly(MapEntry.entry("max", "20")); + assertThat(activeRuleDiff.paramDifference().entriesOnlyOnRight()).containsExactly(MapEntry.entry("min", "5")); + } + + @Test(expected = IllegalArgumentException.class) + public void fail_on_unknown_left() throws Exception { + comparison.compare("polop", right.getKey()); + } + + @Test(expected = IllegalArgumentException.class) + public void fail_on_unknown_right() throws Exception { + comparison.compare(left.getKey(), "polop"); + } +} diff --git a/server/sonar-server/src/test/java/org/sonar/server/qualityprofile/ws/QProfileCompareActionMediumTest.java b/server/sonar-server/src/test/java/org/sonar/server/qualityprofile/ws/QProfileCompareActionMediumTest.java index 9abcb7a6a10..19aeceeaf9d 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/qualityprofile/ws/QProfileCompareActionMediumTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/qualityprofile/ws/QProfileCompareActionMediumTest.java @@ -115,6 +115,42 @@ public class QProfileCompareActionMediumTest { .execute().assertJson(this.getClass(), "compare_nominal.json"); } + @Test + public void compare_param_on_left() throws Exception { + RuleDto rule1 = createRuleWithParam("xoo", "rule1"); + + QualityProfileDto profile1 = createProfile("xoo", "Profile 1", "xoo-profile-1-01234"); + createActiveRuleWithParam(rule1, profile1, "polop"); + session.commit(); + + QualityProfileDto profile2 = createProfile("xoo", "Profile 2", "xoo-profile-2-12345"); + createActiveRule(rule1, profile2); + session.commit(); + + wsTester.newGetRequest("api/qualityprofiles", "compare") + .setParam("leftKey", profile1.getKey()) + .setParam("rightKey", profile2.getKey()) + .execute().assertJson(this.getClass(), "compare_param_on_left.json"); + } + + @Test + public void compare_param_on_right() throws Exception { + RuleDto rule1 = createRuleWithParam("xoo", "rule1"); + + QualityProfileDto profile1 = createProfile("xoo", "Profile 1", "xoo-profile-1-01234"); + createActiveRule(rule1, profile1); + session.commit(); + + QualityProfileDto profile2 = createProfile("xoo", "Profile 2", "xoo-profile-2-12345"); + createActiveRuleWithParam(rule1, profile2, "polop"); + session.commit(); + + wsTester.newGetRequest("api/qualityprofiles", "compare") + .setParam("leftKey", profile1.getKey()) + .setParam("rightKey", profile2.getKey()) + .execute().assertJson(this.getClass(), "compare_param_on_right.json"); + } + @Test(expected = IllegalArgumentException.class) public void fail_on_missing_left_param() throws Exception { wsTester.newGetRequest("api/qualityprofiles", "compare") diff --git a/server/sonar-server/src/test/resources/org/sonar/server/qualityprofile/ws/QProfileCompareActionMediumTest/compare_param_on_left.json b/server/sonar-server/src/test/resources/org/sonar/server/qualityprofile/ws/QProfileCompareActionMediumTest/compare_param_on_left.json new file mode 100644 index 00000000000..afe92f1d381 --- /dev/null +++ b/server/sonar-server/src/test/resources/org/sonar/server/qualityprofile/ws/QProfileCompareActionMediumTest/compare_param_on_left.json @@ -0,0 +1,32 @@ +{ + "left" : { + "key" : "xoo-profile-1-01234", + "name" : "Profile 1" + }, + "right" : { + "key" : "xoo-profile-2-12345", + "name" : "Profile 2" + }, + "same" : [], + "inLeft" : [], + "inRight" : [], + "modified" : [ + { + "key" : "blah:rule1", + "pluginKey" : "blah", + "pluginName" : "Blah", + "languageKey": "xoo", + "languageName": "Xoo", + "name" : "Rule1", + "left" : { + "severity" : "BLOCKER", + "params" : { + "param_rule1" : "polop" + } + }, + "right" : { + "severity" : "BLOCKER" + } + } + ] +} diff --git a/server/sonar-server/src/test/resources/org/sonar/server/qualityprofile/ws/QProfileCompareActionMediumTest/compare_param_on_right.json b/server/sonar-server/src/test/resources/org/sonar/server/qualityprofile/ws/QProfileCompareActionMediumTest/compare_param_on_right.json new file mode 100644 index 00000000000..ec902279be3 --- /dev/null +++ b/server/sonar-server/src/test/resources/org/sonar/server/qualityprofile/ws/QProfileCompareActionMediumTest/compare_param_on_right.json @@ -0,0 +1,32 @@ +{ + "left" : { + "key" : "xoo-profile-1-01234", + "name" : "Profile 1" + }, + "right" : { + "key" : "xoo-profile-2-12345", + "name" : "Profile 2" + }, + "same" : [], + "inLeft" : [], + "inRight" : [], + "modified" : [ + { + "key" : "blah:rule1", + "pluginKey" : "blah", + "pluginName" : "Blah", + "languageKey": "xoo", + "languageName": "Xoo", + "name" : "Rule1", + "left" : { + "severity" : "BLOCKER" + }, + "right" : { + "severity" : "BLOCKER", + "params" : { + "param_rule1" : "polop" + } + } + } + ] +} -- 2.39.5