From 65e3e57e3ea7212f0b01cffb54d4fb4a7f74d469 Mon Sep 17 00:00:00 2001 From: Godin Date: Thu, 2 Dec 2010 19:27:22 +0000 Subject: [PATCH] SONAR-1450: Compare violations using checksums --- .../org/sonar/plugins/core/CorePlugin.java | 1 + .../timemachine/PastViolationsLoader.java | 51 +++++++ .../ViolationPersisterDecorator.java | 134 +++++++++++++++--- .../ViolationPersisterDecoratorTest.java | 95 +++++++++++++ .../sonar/batch/index/ViolationPersister.java | 1 + 5 files changed, 260 insertions(+), 22 deletions(-) create mode 100644 plugins/sonar-core-plugin/src/main/java/org/sonar/plugins/core/timemachine/PastViolationsLoader.java create mode 100644 plugins/sonar-core-plugin/src/test/java/org/sonar/plugins/core/timemachine/ViolationPersisterDecoratorTest.java diff --git a/plugins/sonar-core-plugin/src/main/java/org/sonar/plugins/core/CorePlugin.java b/plugins/sonar-core-plugin/src/main/java/org/sonar/plugins/core/CorePlugin.java index 17dfc760380..406d9aa6bbb 100644 --- a/plugins/sonar-core-plugin/src/main/java/org/sonar/plugins/core/CorePlugin.java +++ b/plugins/sonar-core-plugin/src/main/java/org/sonar/plugins/core/CorePlugin.java @@ -215,6 +215,7 @@ public class CorePlugin implements Plugin { extensions.add(PastMeasuresLoader.class); extensions.add(TimeMachineConfiguration.class); extensions.add(VariationDecorator.class); + extensions.add(PastViolationsLoader.class); extensions.add(ViolationPersisterDecorator.class); extensions.add(NewViolationsDecorator.class); diff --git a/plugins/sonar-core-plugin/src/main/java/org/sonar/plugins/core/timemachine/PastViolationsLoader.java b/plugins/sonar-core-plugin/src/main/java/org/sonar/plugins/core/timemachine/PastViolationsLoader.java new file mode 100644 index 00000000000..e4217bbc5c2 --- /dev/null +++ b/plugins/sonar-core-plugin/src/main/java/org/sonar/plugins/core/timemachine/PastViolationsLoader.java @@ -0,0 +1,51 @@ +package org.sonar.plugins.core.timemachine; + +import org.sonar.api.BatchExtension; +import org.sonar.api.database.DatabaseSession; +import org.sonar.api.database.model.RuleFailureModel; +import org.sonar.api.database.model.Snapshot; +import org.sonar.api.database.model.SnapshotSource; +import org.sonar.api.resources.Resource; +import org.sonar.batch.index.ResourcePersister; + +import java.util.Collections; +import java.util.List; + +public class PastViolationsLoader implements BatchExtension { + + private DatabaseSession session; + private ResourcePersister resourcePersister; + + public PastViolationsLoader(DatabaseSession session, ResourcePersister resourcePersister) { + this.session = session; + this.resourcePersister = resourcePersister; + } + + public Snapshot getPreviousLastSnapshot(Resource resource) { + Snapshot snapshot = resourcePersister.getSnapshot(resource); + return resourcePersister.getLastSnapshot(snapshot, true); + } + + public List getPastViolations(Snapshot previousLastSnapshot) { + if (previousLastSnapshot == null) { + return Collections.emptyList(); + } + return session.getResults(RuleFailureModel.class, + "snapshotId", previousLastSnapshot.getId()); + } + + public SnapshotSource getPastSource(Snapshot previousLastSnapshot) { + if (previousLastSnapshot == null) { + return null; + } + return session.getSingleResult(SnapshotSource.class, + "snapshotId", previousLastSnapshot.getId()); + } + + public SnapshotSource getSource(Resource resource) { + Snapshot snapshot = resourcePersister.getSnapshot(resource); + return session.getSingleResult(SnapshotSource.class, + "snapshotId", snapshot.getId()); + } + +} 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 5da0d7fd894..37322683199 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 @@ -1,30 +1,45 @@ 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 org.apache.commons.codec.digest.DigestUtils; +import org.apache.commons.io.IOUtils; +import org.apache.commons.lang.StringUtils; +import org.codehaus.plexus.util.StringInputStream; import org.sonar.api.batch.Decorator; import org.sonar.api.batch.DecoratorBarriers; import org.sonar.api.batch.DecoratorContext; import org.sonar.api.batch.DependsUpon; -import org.sonar.api.database.DatabaseSession; import org.sonar.api.database.model.RuleFailureModel; import org.sonar.api.database.model.Snapshot; +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.batch.index.ResourcePersister; +import org.sonar.api.utils.SonarException; import org.sonar.batch.index.ViolationPersister; +import java.io.IOException; +import java.util.Collection; +import java.util.Collections; import java.util.List; @DependsUpon(DecoratorBarriers.END_OF_VIOLATIONS_GENERATION) public class ViolationPersisterDecorator implements Decorator { - private DatabaseSession session; - private ResourcePersister resourcePersister; + private RuleFinder ruleFinder; + private PastViolationsLoader pastViolationsLoader; private ViolationPersister violationPersister; - public ViolationPersisterDecorator(DatabaseSession session, ResourcePersister resourcePersister, ViolationPersister violationPersister) { - this.session = session; - this.resourcePersister = resourcePersister; + private List checksums = Lists.newArrayList(); + private List pastChecksums = Lists.newArrayList(); + + public ViolationPersisterDecorator(RuleFinder ruleFinder, PastViolationsLoader pastViolationsLoader, ViolationPersister violationPersister) { + this.ruleFinder = ruleFinder; + this.pastViolationsLoader = pastViolationsLoader; this.violationPersister = violationPersister; } @@ -32,28 +47,103 @@ public class ViolationPersisterDecorator implements Decorator { return true; } - private RuleFailureModel selectPreviousViolation(Violation violation) { - Snapshot snapshot = resourcePersister.getSnapshot(violation.getResource()); - Snapshot previousLastSnapshot = resourcePersister.getLastSnapshot(snapshot, true); - if (previousLastSnapshot == null) { + public void decorate(Resource resource, DecoratorContext context) { + Snapshot previousLastSnapshot = pastViolationsLoader.getPreviousLastSnapshot(resource); + // Load past violations + List pastViolations = pastViolationsLoader.getPastViolations(previousLastSnapshot); + // Load past source and calculate checksums + pastChecksums = getChecksums(pastViolationsLoader.getPastSource(previousLastSnapshot)); + // Load current source and calculate checksums + checksums = getChecksums(pastViolationsLoader.getSource(resource)); + // Save violations + compareWithPastViolations(context, pastViolations); + // Clear caches + checksums.clear(); + pastChecksums.clear(); + } + + private void compareWithPastViolations(DecoratorContext context, List pastViolations) { + Multimap pastViolationsByRule = LinkedHashMultimap.create(); + for (RuleFailureModel pastViolation : pastViolations) { + Rule rule = ruleFinder.findById(pastViolation.getRuleId()); + pastViolationsByRule.put(rule, pastViolation); + } + // for each violation, search equivalent past violation + for (Violation violation : context.getViolations()) { + RuleFailureModel pastViolation = selectPastViolation(violation, pastViolationsByRule); + violationPersister.saveOrUpdateViolation(context.getProject(), violation, pastViolation); + } + } + + private List getChecksums(SnapshotSource source) { + return source == null ? Collections. emptyList() : getChecksums(source.getData()); + } + + static List getChecksums(String data) { + List result = Lists.newArrayList(); + try { + List lines = IOUtils.readLines(new StringInputStream(data)); + for (String line : lines) { + String reducedLine = StringUtils.replaceChars(line, "\t\n\r ", ""); + result.add(DigestUtils.md5Hex(reducedLine)); + } + } catch (IOException e) { + throw new SonarException("Unable to calculate checksums", e); + } + return result; + } + + private String getChecksumForLine(List checksums, Integer line) { + if (line == null || line < 1 || line > checksums.size()) { return null; } - // Can be several violations on line with same message: for example - "'3' is a magic number" - List models = session.getResults(RuleFailureModel.class, - "snapshotId", previousLastSnapshot.getId(), - "line", violation.getLineId(), - "message", violation.getMessage()); - if (models != null && !models.isEmpty()) { - return models.get(0); + return checksums.get(line - 1); + } + + /** + * Search for past violation. + */ + RuleFailureModel selectPastViolation(Violation violation, Multimap pastViolationsByRule) { + // skip violation, if there is no past violations with same rule + if (!pastViolationsByRule.containsKey(violation.getRule())) { + return null; + } + Collection pastViolations = pastViolationsByRule.get(violation.getRule()); + RuleFailureModel found = selectPastViolationUsingLine(violation, pastViolations); + if (found == null) { + found = selectPastViolationUsingChecksum(violation, pastViolations); + } + return found; + } + + /** + * Search for past violation with same message and line. + */ + RuleFailureModel selectPastViolationUsingLine(Violation violation, Collection pastViolations) { + for (RuleFailureModel pastViolation : pastViolations) { + if (violation.getLineId() == pastViolation.getLine() && StringUtils.equals(violation.getMessage(), pastViolation.getMessage())) { + return pastViolation; + } } return null; } - public void decorate(Resource resource, DecoratorContext context) { - for (Violation violation : context.getViolations()) { - RuleFailureModel previousViolation = selectPreviousViolation(violation); - violationPersister.saveOrUpdateViolation(context.getProject(), violation, previousViolation); + /** + * Search for past violation with same message and checksum. + */ + RuleFailureModel selectPastViolationUsingChecksum(Violation violation, 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 = getChecksumForLine(pastChecksums, pastViolation.getLine()); + if (StringUtils.equals(checksum, pastChecksum) && StringUtils.equals(violation.getMessage(), pastViolation.getMessage())) { + return pastViolation; + } } + return null; } @Override 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 new file mode 100644 index 00000000000..2156c2f516d --- /dev/null +++ b/plugins/sonar-core-plugin/src/test/java/org/sonar/plugins/core/timemachine/ViolationPersisterDecoratorTest.java @@ -0,0 +1,95 @@ +package org.sonar.plugins.core.timemachine; + +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.LinkedHashMultimap; +import com.google.common.collect.Multimap; +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; + +public class ViolationPersisterDecoratorTest { + + private ViolationPersisterDecorator decorator; + + @Before + public void setUp() { + decorator = new ViolationPersisterDecorator(null, null, null); + } + + @Test + public void testGetChecksums() { + List crlf = ViolationPersisterDecorator.getChecksums("Hello\r\nWorld"); + List lf = ViolationPersisterDecorator.getChecksums("Hello\nWorld"); + assertThat(crlf.size(), is(2)); + assertThat(crlf.get(0), not(equalTo(crlf.get(1)))); + assertThat(lf, equalTo(crlf)); + + List spaces = ViolationPersisterDecorator.getChecksums("" + + "\tvoid method() {\n" + + " void method() {"); + assertThat(spaces.get(0), equalTo(spaces.get(1))); + } + + @Test + public void sameRuleLineMessage() { + Rule rule = Rule.create().setKey("rule"); + Violation violation = Violation.create(rule, null) + .setLineId(1).setMessage("message"); + + RuleFailureModel pastViolation = newPastViolation(rule, 1, "message"); + + Multimap pastViolationsByRule = LinkedHashMultimap.create(); + pastViolationsByRule.put(rule, pastViolation); + + RuleFailureModel found = decorator.selectPastViolation(violation, pastViolationsByRule); + assertThat(found, equalTo(pastViolation)); + } + + @Test + public void differentLine() { + Rule rule = Rule.create().setKey("rule"); + Violation violation = Violation.create(rule, null) + .setLineId(1).setMessage("message"); + + RuleFailureModel pastViolation = newPastViolation(rule, 2, "message"); + + Multimap pastViolationsByRule = LinkedHashMultimap.create(); + pastViolationsByRule.put(rule, pastViolation); + + RuleFailureModel found = decorator.selectPastViolation(violation, pastViolationsByRule); + assertThat(found, nullValue()); + } + + @Test + public void differentRule() { + Rule rule = Rule.create().setKey("rule"); + Violation violation = Violation.create(rule, null) + .setLineId(1).setMessage("message"); + + Rule anotherRule = Rule.create().setKey("anotherRule"); + RuleFailureModel pastViolation = newPastViolation(anotherRule, 1, "message"); + + Multimap pastViolationsByRule = LinkedHashMultimap.create(); + pastViolationsByRule.put(anotherRule, pastViolation); + + RuleFailureModel found = decorator.selectPastViolation(violation, pastViolationsByRule); + assertThat(found, nullValue()); + } + + private RuleFailureModel newPastViolation(Rule rule, Integer line, String message) { + RuleFailureModel pastViolation = new RuleFailureModel(); + pastViolation.setLine(line); + pastViolation.setMessage(message); + return pastViolation; + } + +} diff --git a/sonar-batch/src/main/java/org/sonar/batch/index/ViolationPersister.java b/sonar-batch/src/main/java/org/sonar/batch/index/ViolationPersister.java index 37c6fdc8b57..59d76c2ed07 100644 --- a/sonar-batch/src/main/java/org/sonar/batch/index/ViolationPersister.java +++ b/sonar-batch/src/main/java/org/sonar/batch/index/ViolationPersister.java @@ -50,6 +50,7 @@ public final class ViolationPersister { Snapshot snapshot = resourcePersister.saveResource(project, violation.getResource()); if (model != null) { // update + model = session.reattach(RuleFailureModel.class, model.getId()); model = mergeModel(violation, model); } else { // insert -- 2.39.5