From: Freddy Mallet Date: Fri, 22 Apr 2011 20:53:26 +0000 (+0200) Subject: SONAR-2344 Improve the algorithm in charge to define/update the creation date of... X-Git-Tag: 2.8~146 X-Git-Url: https://source.dussan.org/?a=commitdiff_plain;h=db1f420129792b3da40d85903b993c5b4bd74d76;p=sonarqube.git SONAR-2344 Improve the algorithm in charge to define/update the creation date of each violation --- diff --git a/plugins/sonar-core-plugin/src/main/java/org/sonar/plugins/core/timemachine/ViolationPersisterDecorator.java b/plugins/sonar-core-plugin/src/main/java/org/sonar/plugins/core/timemachine/ViolationPersisterDecorator.java index 59ad0a168ca..202838755a3 100644 --- a/plugins/sonar-core-plugin/src/main/java/org/sonar/plugins/core/timemachine/ViolationPersisterDecorator.java +++ b/plugins/sonar-core-plugin/src/main/java/org/sonar/plugins/core/timemachine/ViolationPersisterDecorator.java @@ -19,33 +19,38 @@ */ package org.sonar.plugins.core.timemachine; -import com.google.common.collect.LinkedHashMultimap; -import com.google.common.collect.Lists; -import com.google.common.collect.Multimap; +import java.io.IOException; +import java.util.Collection; +import java.util.Collections; +import java.util.IdentityHashMap; +import java.util.List; +import java.util.Map; + import org.apache.commons.codec.digest.DigestUtils; import org.apache.commons.io.IOUtils; -import org.apache.commons.lang.ObjectUtils; import org.apache.commons.lang.StringUtils; import org.codehaus.plexus.util.StringInputStream; -import org.sonar.api.batch.*; +import org.sonar.api.batch.Decorator; +import org.sonar.api.batch.DecoratorBarriers; +import org.sonar.api.batch.DecoratorContext; +import org.sonar.api.batch.DependedUpon; +import org.sonar.api.batch.DependsUpon; import org.sonar.api.database.model.RuleFailureModel; import org.sonar.api.database.model.SnapshotSource; import org.sonar.api.resources.Project; import org.sonar.api.resources.Resource; -import org.sonar.api.rules.Rule; -import org.sonar.api.rules.RuleFinder; import org.sonar.api.rules.Violation; import org.sonar.api.utils.SonarException; import org.sonar.batch.components.PastViolationsLoader; import org.sonar.batch.index.ViolationPersister; -import java.io.IOException; -import java.util.Collection; -import java.util.Collections; -import java.util.List; +import com.google.common.collect.LinkedHashMultimap; +import com.google.common.collect.Lists; +import com.google.common.collect.Multimap; @DependsUpon(DecoratorBarriers.END_OF_VIOLATIONS_GENERATION) -@DependedUpon("ViolationPersisterDecorator") /* temporary workaround - see NewViolationsDecorator */ +@DependedUpon("ViolationPersisterDecorator") +/* temporary workaround - see NewViolationsDecorator */ public class ViolationPersisterDecorator implements Decorator { /** @@ -53,14 +58,12 @@ public class ViolationPersisterDecorator implements Decorator { */ private static final String SPACE_CHARS = "\t\n\r "; - private RuleFinder ruleFinder; private PastViolationsLoader pastViolationsLoader; private ViolationPersister violationPersister; - List checksums = Lists.newArrayList(); + List checksums; - public ViolationPersisterDecorator(RuleFinder ruleFinder, PastViolationsLoader pastViolationsLoader, ViolationPersister violationPersister) { - this.ruleFinder = ruleFinder; + public ViolationPersisterDecorator(PastViolationsLoader pastViolationsLoader, ViolationPersister violationPersister) { this.pastViolationsLoader = pastViolationsLoader; this.violationPersister = violationPersister; } @@ -73,40 +76,125 @@ public class ViolationPersisterDecorator implements Decorator { if (context.getViolations().isEmpty()) { return; } + // Load new violations + List newViolations = context.getViolations(); + // Load past violations List pastViolations = pastViolationsLoader.getPastViolations(resource); - // Load current source and calculate checksums + + // Load current source code and calculate checksums for each line checksums = getChecksums(pastViolationsLoader.getSource(resource)); - // Save violations - compareWithPastViolations(context, pastViolations); + + // Map new violations with old ones + Map violationMap = mapViolations(newViolations, pastViolations); + + for (Violation newViolation : newViolations) { + String checksum = getChecksumForLine(checksums, newViolation.getLineId()); + violationPersister.saveViolation(context.getProject(), newViolation, violationMap.get(newViolation), checksum); + } + violationPersister.commit(); // Clear cache checksums.clear(); } - private void compareWithPastViolations(DecoratorContext context, List pastViolations) { - Multimap pastViolationsByRule = LinkedHashMultimap.create(); + Map mapViolations(List newViolations, List pastViolations) { + Map violationMap = new IdentityHashMap(); + + Multimap pastViolationsByRule = LinkedHashMultimap.create(); for (RuleFailureModel pastViolation : pastViolations) { - Rule rule = ruleFinder.findById(pastViolation.getRuleId()); - pastViolationsByRule.put(rule, pastViolation); + pastViolationsByRule.put(pastViolation.getRuleId(), pastViolation); } - // for each violation, search equivalent past violation - for (Violation violation : context.getViolations()) { - RuleFailureModel pastViolation = selectPastViolation(violation, pastViolationsByRule); - if (pastViolation != null) { - // remove violation, since would be updated and shouldn't affect other violations anymore - pastViolationsByRule.remove(violation.getRule(), pastViolation); + + // Try first an exact matching : same rule, same message, same line and same checkum + for (Violation newViolation : newViolations) { + mapViolation(newViolation, + findPastViolationWithSameLineAndChecksumAndMessage(newViolation, pastViolationsByRule.get(newViolation.getRule().getId())), + pastViolationsByRule, violationMap); + } + + // If each new violation matches an old one we can stop the matching mechanism + if (violationMap.size() != newViolations.size()) { + + // Try then to match violations on same rule with same message and with same checkum + for (Violation newViolation : newViolations) { + if (isNotAlreadyMapped(newViolation, violationMap)) { + mapViolation(newViolation, + findPastViolationWithSameChecksumAndMessage(newViolation, pastViolationsByRule.get(newViolation.getRule().getId())), + pastViolationsByRule, violationMap); + } + } + + // Try then to match violations on same rule with same line and with same message + for (Violation newViolation : newViolations) { + if (isNotAlreadyMapped(newViolation, violationMap)) { + mapViolation(newViolation, + findPastViolationWithSameLineAndMessage(newViolation, pastViolationsByRule.get(newViolation.getRule().getId())), + pastViolationsByRule, violationMap); + } } - String checksum = getChecksumForLine(checksums, violation.getLineId()); - violationPersister.saveViolation(context.getProject(), violation, pastViolation, checksum); } - violationPersister.commit(); + + return violationMap; + } + + private final boolean isNotAlreadyMapped(Violation newViolation, Map violationMap) { + return violationMap.get(newViolation) == null; + } + + private RuleFailureModel findPastViolationWithSameLineAndMessage(Violation newViolation, Collection pastViolations) { + for (RuleFailureModel pastViolation : pastViolations) { + if (isSameLine(newViolation, pastViolation) && isSameMessage(newViolation, pastViolation)) { + return pastViolation; + } + } + return null; + } + + private RuleFailureModel findPastViolationWithSameChecksumAndMessage(Violation newViolation, Collection pastViolations) { + for (RuleFailureModel pastViolation : pastViolations) { + if (isSameChecksum(newViolation, pastViolation) && isSameMessage(newViolation, pastViolation)) { + return pastViolation; + } + } + return null; + } + + private RuleFailureModel findPastViolationWithSameLineAndChecksumAndMessage(Violation newViolation, + Collection pastViolations) { + for (RuleFailureModel pastViolation : pastViolations) { + if (isSameLine(newViolation, pastViolation) && isSameChecksum(newViolation, pastViolation) + && isSameMessage(newViolation, pastViolation)) { + return pastViolation; + } + } + return null; + } + + private final boolean isSameChecksum(Violation newViolation, RuleFailureModel pastViolation) { + return pastViolation.getChecksum().equals(getChecksumForLine(checksums, newViolation.getLineId())); + } + + private final boolean isSameLine(Violation newViolation, RuleFailureModel pastViolation) { + return pastViolation.getLine() == newViolation.getLineId(); + } + + private final boolean isSameMessage(Violation newViolation, RuleFailureModel pastViolation) { + return StringUtils.equals(RuleFailureModel.abbreviateMessage(newViolation.getMessage()), pastViolation.getMessage()); + } + + private void mapViolation(Violation newViolation, RuleFailureModel pastViolation, + Multimap pastViolationsByRule, Map violationMap) { + if (pastViolation != null) { + pastViolationsByRule.remove(newViolation.getRule().getId(), pastViolation); + violationMap.put(newViolation, pastViolation); + } } /** * @return checksums, never null */ private List getChecksums(SnapshotSource source) { - return source == null || source.getData() == null ? Collections.emptyList() : getChecksums(source.getData()); + return source == null || source.getData() == null ? Collections. emptyList() : getChecksums(source.getData()); } static List getChecksums(String data) { @@ -119,7 +207,7 @@ public class ViolationPersisterDecorator implements Decorator { } } catch (IOException e) { throw new SonarException("Unable to calculate checksums", e); - + } finally { IOUtils.closeQuietly(stream); } @@ -141,53 +229,6 @@ public class ViolationPersisterDecorator implements Decorator { return checksums.get(line - 1); } - /** - * Search for past violation. - */ - RuleFailureModel selectPastViolation(Violation violation, Multimap pastViolationsByRule) { - Collection pastViolations = pastViolationsByRule.get(violation.getRule()); - if (pastViolations==null || pastViolations.isEmpty()) { - // skip violation, if there is no past violations with same rule - return null; - } - String dbFormattedMessage = RuleFailureModel.abbreviateMessage(violation.getMessage()); - RuleFailureModel found = selectPastViolationUsingLine(violation, dbFormattedMessage, pastViolations); - if (found == null) { - found = selectPastViolationUsingChecksum(violation, dbFormattedMessage, pastViolations); - } - return found; - } - - /** - * Search for past violation with same message and line. - */ - private RuleFailureModel selectPastViolationUsingLine(Violation violation, String dbFormattedMessage, Collection pastViolations) { - for (RuleFailureModel pastViolation : pastViolations) { - if (ObjectUtils.equals(violation.getLineId(), pastViolation.getLine()) && StringUtils.equals(dbFormattedMessage, pastViolation.getMessage())) { - return pastViolation; - } - } - return null; - } - - /** - * Search for past violation with same message and checksum. - */ - private RuleFailureModel selectPastViolationUsingChecksum(Violation violation, String dbFormattedMessage, Collection pastViolations) { - String checksum = getChecksumForLine(checksums, violation.getLineId()); - // skip violation, which not attached to line - if (checksum == null) { - return null; - } - for (RuleFailureModel pastViolation : pastViolations) { - String pastChecksum = pastViolation.getChecksum(); - if (StringUtils.equals(checksum, pastChecksum) && StringUtils.equals(dbFormattedMessage, pastViolation.getMessage())) { - return pastViolation; - } - } - return null; - } - @Override public String toString() { return getClass().getSimpleName(); diff --git a/plugins/sonar-core-plugin/src/test/java/org/sonar/plugins/core/timemachine/ViolationPersisterDecoratorTest.java b/plugins/sonar-core-plugin/src/test/java/org/sonar/plugins/core/timemachine/ViolationPersisterDecoratorTest.java index 0c715997b39..44538c85633 100644 --- a/plugins/sonar-core-plugin/src/test/java/org/sonar/plugins/core/timemachine/ViolationPersisterDecoratorTest.java +++ b/plugins/sonar-core-plugin/src/test/java/org/sonar/plugins/core/timemachine/ViolationPersisterDecoratorTest.java @@ -19,21 +19,22 @@ */ package org.sonar.plugins.core.timemachine; -import com.google.common.collect.LinkedHashMultimap; -import com.google.common.collect.Multimap; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.not; +import static org.hamcrest.Matchers.nullValue; +import static org.junit.Assert.assertThat; + +import java.util.List; +import java.util.Map; + import org.junit.Before; import org.junit.Test; import org.sonar.api.database.model.RuleFailureModel; import org.sonar.api.rules.Rule; import org.sonar.api.rules.Violation; -import java.util.List; - -import static org.hamcrest.Matchers.equalTo; -import static org.hamcrest.Matchers.is; -import static org.hamcrest.Matchers.not; -import static org.hamcrest.Matchers.nullValue; -import static org.junit.Assert.assertThat; +import com.google.common.collect.Lists; public class ViolationPersisterDecoratorTest { @@ -41,7 +42,7 @@ public class ViolationPersisterDecoratorTest { @Before public void setUp() { - decorator = new ViolationPersisterDecorator(null, null, null); + decorator = new ViolationPersisterDecorator(null, null); } @Test @@ -59,95 +60,97 @@ public class ViolationPersisterDecoratorTest { } @Test - public void sameRuleLineMessage() { - Rule rule = Rule.create().setKey("rule"); - rule.setId(50); - Violation violation = Violation.create(rule, null) - .setLineId(1).setMessage("message"); - - RuleFailureModel pastViolation = newPastViolation(rule, 1, "message"); + public void checksumShouldHaveGreaterPriorityThanLine() { + RuleFailureModel pastViolation1 = newPastViolation("message", 1, 50, "checksum1"); + RuleFailureModel pastViolation2 = newPastViolation("message", 3, 50, "checksum2"); - Multimap pastViolationsByRule = LinkedHashMultimap.create(); - pastViolationsByRule.put(rule, pastViolation); + Violation newViolation1 = newViolation("message", 3, 50, "checksum1"); + Violation newViolation2 = newViolation("message", 5, 50, "checksum2"); - RuleFailureModel found = decorator.selectPastViolation(violation, pastViolationsByRule); - assertThat(found, equalTo(pastViolation)); + Map mapping = decorator.mapViolations(Lists.newArrayList(newViolation1, newViolation2), + Lists.newArrayList(pastViolation1, pastViolation2)); + assertThat(mapping.get(newViolation1), equalTo(pastViolation1)); + assertThat(mapping.get(newViolation2), equalTo(pastViolation2)); } @Test - public void sameRuleAndMessageButDifferentLine() { - Rule rule = Rule.create().setKey("rule"); - rule.setId(50); - Violation violation = Violation.create(rule, null) - .setLineId(1).setMessage("message"); - decorator.checksums = ViolationPersisterDecorator.getChecksums("violation"); - - RuleFailureModel pastViolation = newPastViolation(rule, 2, "message"); - pastViolation.setChecksum(ViolationPersisterDecorator.getChecksum("violation")); + public void sameRuleAndLineMessage() { + Violation newViolation = newViolation("message", 1, 50, "checksum1"); + RuleFailureModel pastViolation = newPastViolation("message", 1, 50, "checksum2"); - Multimap pastViolationsByRule = LinkedHashMultimap.create(); - pastViolationsByRule.put(rule, pastViolation); - - RuleFailureModel found = decorator.selectPastViolation(violation, pastViolationsByRule); - assertThat(found, equalTo(pastViolation)); + Map mapping = decorator.mapViolations(Lists.newArrayList(newViolation), Lists.newArrayList(pastViolation)); + assertThat(mapping.get(newViolation), equalTo(pastViolation)); } @Test - public void shouldCreateNewViolation() { - Rule rule = Rule.create().setKey("rule"); - Violation violation = Violation.create(rule, null) - .setLineId(1).setMessage("message"); + public void sameRuleAndMessageAndChecksumButDifferentLine() { + Violation newViolation = newViolation("message", 1, 50, "checksum1"); + RuleFailureModel pastViolation = newPastViolation("message", 2, 50, "checksum1"); - RuleFailureModel pastViolation = newPastViolation(rule, 2, "message"); + Map mapping = decorator.mapViolations(Lists.newArrayList(newViolation), Lists.newArrayList(pastViolation)); + assertThat(mapping.get(newViolation), equalTo(pastViolation)); + } - Multimap pastViolationsByRule = LinkedHashMultimap.create(); - pastViolationsByRule.put(rule, pastViolation); + @Test + public void shouldCreateNewViolationWhenSameRuleSameMessageButDifferentLineAndChecksum() { + Violation newViolation = newViolation("message", 1, 50, "checksum1"); + RuleFailureModel pastViolation = newPastViolation("message", 2, 50, "checksum2"); - RuleFailureModel found = decorator.selectPastViolation(violation, pastViolationsByRule); - assertThat(found, nullValue()); + Map mapping = decorator.mapViolations(Lists.newArrayList(newViolation), Lists.newArrayList(pastViolation)); + assertThat(mapping.get(newViolation), is(nullValue())); } @Test public void shouldNotTrackViolationIfDifferentRule() { - Rule rule = Rule.create().setKey("rule"); - rule.setId(50); - Violation violation = Violation.create(rule, null) - .setLineId(1).setMessage("message"); - - Rule otherRule = Rule.create().setKey("anotherRule"); - otherRule.setId(244); - RuleFailureModel pastViolationOnOtherRule = newPastViolation(otherRule, 1, "message"); + Violation newViolation = newViolation("message", 1, 50, "checksum1"); + RuleFailureModel pastViolation = newPastViolation("message", 1, 51, "checksum1"); - Multimap pastViolationsByRule = LinkedHashMultimap.create(); - pastViolationsByRule.put(otherRule, pastViolationOnOtherRule); - - RuleFailureModel found = decorator.selectPastViolation(violation, pastViolationsByRule); - assertThat(found, nullValue()); + Map mapping = decorator.mapViolations(Lists.newArrayList(newViolation), Lists.newArrayList(pastViolation)); + assertThat(mapping.get(newViolation), is(nullValue())); } @Test public void shouldCompareViolationsWithDatabaseFormat() { // violation messages are trimmed and can be abbreviated when persisted in database. // Comparing violation messages must use the same format. - Rule rule = Rule.create().setKey("rule"); - rule.setId(50); - Violation violation = Violation.create(rule, null) - .setLineId(30).setMessage(" message "); // starts and ends with whitespaces + Violation newViolation = newViolation("message", 1, 50, "checksum1"); + RuleFailureModel pastViolation = newPastViolation(" message ", 1, 50, "checksum2"); - RuleFailureModel pastViolation = newPastViolation(rule, 30, "message"); // trimmed in database + Map mapping = decorator.mapViolations(Lists.newArrayList(newViolation), Lists.newArrayList(pastViolation)); + assertThat(mapping.get(newViolation), equalTo(pastViolation)); + } - Multimap pastViolationsByRule = LinkedHashMultimap.create(); - pastViolationsByRule.put(rule, pastViolation); + private Violation newViolation(String message, int lineId, int ruleId) { + Rule rule = Rule.create().setKey("rule"); + rule.setId(ruleId); + Violation violation = Violation.create(rule, null).setLineId(lineId).setMessage(message); + return violation; + } - RuleFailureModel found = decorator.selectPastViolation(violation, pastViolationsByRule); - assertThat(found, equalTo(pastViolation)); + private Violation newViolation(String message, int lineId, int ruleId, String lineChecksum) { + Violation violation = newViolation(message, lineId, ruleId); + if (decorator.checksums == null) { + decorator.checksums = Lists.newArrayListWithExpectedSize(100); + } + for (int i = decorator.checksums.size() - 1; i < lineId; i++) { + decorator.checksums.add(""); + } + decorator.checksums.set(lineId - 1, ViolationPersisterDecorator.getChecksum(lineChecksum)); + return violation; } - private RuleFailureModel newPastViolation(Rule rule, Integer line, String message) { + private RuleFailureModel newPastViolation(String message, int lineId, int ruleId) { RuleFailureModel pastViolation = new RuleFailureModel(); - pastViolation.setLine(line); + pastViolation.setId(lineId + ruleId); + pastViolation.setLine(lineId); pastViolation.setMessage(message); - pastViolation.setRuleId(rule.getId()); + pastViolation.setRuleId(ruleId); + return pastViolation; + } + + private RuleFailureModel newPastViolation(String message, int lineId, int ruleId, String lineChecksum) { + RuleFailureModel pastViolation = newPastViolation(message, lineId, ruleId); + pastViolation.setChecksum(ViolationPersisterDecorator.getChecksum(lineChecksum)); return pastViolation; }