From 6e611257a0ce85043e9d22f4297cbfb03cae508a Mon Sep 17 00:00:00 2001 From: Godin Date: Mon, 29 Nov 2010 15:31:20 +0000 Subject: [PATCH] SONAR-1450: Add support for incremental review of incoming violations * Move getPreviousLastSnapshot from UpdateStatusJob to DefaultResourcePersister * ViolationPersister should allow load and update of violations --- .../org/sonar/batch/index/DefaultIndex.java | 24 ++++++--- .../index/DefaultPersistenceManager.java | 9 +++- .../batch/index/DefaultResourcePersister.java | 16 ++++-- .../sonar/batch/index/PersistenceManager.java | 5 +- .../sonar/batch/index/ResourcePersister.java | 2 + .../sonar/batch/index/ViolationPersister.java | 52 ++++++++++++++----- .../sonar/batch/phases/UpdateStatusJob.java | 16 ++---- .../batch/index/ViolationPersisterTest.java | 35 +++++++++++-- .../batch/phases/UpdateStatusJobTest.java | 7 +-- .../index/ViolationPersisterTest/shared.xml | 1 + ....xml => shouldInsertViolations-result.xml} | 7 +-- .../shouldUpdateViolation-result.xml | 22 ++++++++ 12 files changed, 149 insertions(+), 47 deletions(-) rename sonar-batch/src/test/resources/org/sonar/batch/index/ViolationPersisterTest/{shouldSaveViolations-result.xml => shouldInsertViolations-result.xml} (89%) create mode 100644 sonar-batch/src/test/resources/org/sonar/batch/index/ViolationPersisterTest/shouldUpdateViolation-result.xml diff --git a/sonar-batch/src/main/java/org/sonar/batch/index/DefaultIndex.java b/sonar-batch/src/main/java/org/sonar/batch/index/DefaultIndex.java index 927177d553e..55dbfa90c64 100644 --- a/sonar-batch/src/main/java/org/sonar/batch/index/DefaultIndex.java +++ b/sonar-batch/src/main/java/org/sonar/batch/index/DefaultIndex.java @@ -28,6 +28,7 @@ import org.slf4j.LoggerFactory; import org.sonar.api.batch.Event; import org.sonar.api.batch.SonarIndex; import org.sonar.api.database.model.ResourceModel; +import org.sonar.api.database.model.RuleFailureModel; import org.sonar.api.design.Dependency; import org.sonar.api.measures.*; import org.sonar.api.profiles.RulesProfile; @@ -64,6 +65,7 @@ public final class DefaultIndex extends SonarIndex { private Set dependencies = Sets.newHashSet(); private Map> outgoingDependenciesByResource = Maps.newHashMap(); private Map> incomingDependenciesByResource = Maps.newHashMap(); + private Map> violationsByResource = Maps.newHashMap(); private ProjectTree projectTree; public DefaultIndex(PersistenceManager persistence, DefaultResourceCreationLock lock, ProjectTree projectTree, MetricFinder metricFinder) { @@ -131,7 +133,6 @@ public final class DefaultIndex extends SonarIndex { lock.unlock(); } - /** * Does nothing if the resource is already registered. */ @@ -196,7 +197,6 @@ public final class DefaultIndex extends SonarIndex { return excluded; } - public List getChildren(Resource resource) { List children = Lists.newArrayList(); Bucket bucket = buckets.get(resource); @@ -262,7 +262,6 @@ public final class DefaultIndex extends SonarIndex { } } - // // // @@ -270,6 +269,7 @@ public final class DefaultIndex extends SonarIndex { // // // + public Dependency addDependency(Dependency dependency) { Dependency existingDep = getEdge(dependency.getFrom(), dependency.getTo()); if (existingDep != null) { @@ -356,7 +356,6 @@ public final class DefaultIndex extends SonarIndex { return result; } - // // // @@ -402,8 +401,21 @@ public final class DefaultIndex extends SonarIndex { } private void doAddViolation(Violation violation, Bucket bucket) { + Resource resource = violation.getResource(); + if (!violationsByResource.containsKey(resource)) { + violationsByResource.put(resource, persistence.loadPreviousViolations(resource)); + } + RuleFailureModel found = null; + List pastViolations = violationsByResource.get(resource); + for (RuleFailureModel pastViolation : pastViolations) { + // TODO Compare pastViolation to violation + if (pastViolation.getLine() == violation.getLineId() && StringUtils.equals(pastViolation.getMessage(), violation.getMessage())) { + found = pastViolation; + break; + } + } bucket.addViolation(violation); - persistence.saveViolation(currentProject, violation); + persistence.saveOrUpdateViolation(currentProject, violation, found); } // @@ -413,6 +425,7 @@ public final class DefaultIndex extends SonarIndex { // // // + public void addLink(ProjectLink link) { persistence.saveLink(currentProject, link); } @@ -421,7 +434,6 @@ public final class DefaultIndex extends SonarIndex { persistence.deleteLink(currentProject, key); } - // // // diff --git a/sonar-batch/src/main/java/org/sonar/batch/index/DefaultPersistenceManager.java b/sonar-batch/src/main/java/org/sonar/batch/index/DefaultPersistenceManager.java index f03147a4ede..2bf06968097 100644 --- a/sonar-batch/src/main/java/org/sonar/batch/index/DefaultPersistenceManager.java +++ b/sonar-batch/src/main/java/org/sonar/batch/index/DefaultPersistenceManager.java @@ -20,6 +20,7 @@ package org.sonar.batch.index; import org.sonar.api.batch.Event; +import org.sonar.api.database.model.RuleFailureModel; import org.sonar.api.database.model.Snapshot; import org.sonar.api.design.Dependency; import org.sonar.api.measures.Measure; @@ -86,8 +87,12 @@ public final class DefaultPersistenceManager implements PersistenceManager { dependencyPersister.saveDependency(project, dependency, parentDependency); } - public void saveViolation(Project project, Violation violation) { - violationPersister.saveViolation(project, violation); + public List loadPreviousViolations(Resource resource) { + return violationPersister.getPreviousViolations(resource); + } + + public void saveOrUpdateViolation(Project project, Violation violation, RuleFailureModel oldModel) { + violationPersister.saveOrUpdateViolation(project, violation, oldModel); } public void saveLink(Project project, ProjectLink link) { diff --git a/sonar-batch/src/main/java/org/sonar/batch/index/DefaultResourcePersister.java b/sonar-batch/src/main/java/org/sonar/batch/index/DefaultResourcePersister.java index e4215524033..05f5ef18803 100644 --- a/sonar-batch/src/main/java/org/sonar/batch/index/DefaultResourcePersister.java +++ b/sonar-batch/src/main/java/org/sonar/batch/index/DefaultResourcePersister.java @@ -33,6 +33,7 @@ import org.sonar.api.utils.SonarException; import javax.persistence.NonUniqueResultException; import javax.persistence.Query; + import java.util.Iterator; import java.util.List; import java.util.Map; @@ -159,7 +160,7 @@ public final class DefaultResourcePersister implements ResourcePersister { model = session.save(model); resource.setId(model.getId()); // TODO to be removed - Snapshot parentSnapshot = (Snapshot)ObjectUtils.defaultIfNull(getSnapshot(resource.getParent()), projectSnapshot); + Snapshot parentSnapshot = (Snapshot) ObjectUtils.defaultIfNull(getSnapshot(resource.getParent()), projectSnapshot); Snapshot snapshot = new Snapshot(model, parentSnapshot); snapshot = session.save(snapshot); session.commit(); @@ -167,17 +168,24 @@ public final class DefaultResourcePersister implements ResourcePersister { return snapshot; } + public Snapshot getPreviousLastSnapshot(Snapshot snapshot) { + Query query = session.createQuery( + "SELECT s FROM " + Snapshot.class.getSimpleName() + " s " + + "WHERE s.last=true AND s.resourceId=:resourceId"); + query.setParameter("resourceId", snapshot.getResourceId()); + return session.getSingleResult(query, null); + } + public void clear() { // we keep cache of projects for (Iterator> it = snapshotsByResource.entrySet().iterator(); it.hasNext();) { Map.Entry entry = it.next(); - if (!ResourceUtils.isSet(entry.getKey())) { + if ( !ResourceUtils.isSet(entry.getKey())) { it.remove(); } } } - private ResourceModel findOrCreateModel(Resource resource) { ResourceModel model; try { @@ -225,7 +233,7 @@ public final class DefaultResourcePersister implements ResourcePersister { if (StringUtils.isNotBlank(resource.getDescription())) { model.setDescription(resource.getDescription()); } - if (!ResourceUtils.isLibrary(resource)) { + if ( !ResourceUtils.isLibrary(resource)) { model.setScope(resource.getScope()); model.setQualifier(resource.getQualifier()); } diff --git a/sonar-batch/src/main/java/org/sonar/batch/index/PersistenceManager.java b/sonar-batch/src/main/java/org/sonar/batch/index/PersistenceManager.java index f53248feff2..ebb3d2ea152 100644 --- a/sonar-batch/src/main/java/org/sonar/batch/index/PersistenceManager.java +++ b/sonar-batch/src/main/java/org/sonar/batch/index/PersistenceManager.java @@ -20,6 +20,7 @@ package org.sonar.batch.index; import org.sonar.api.batch.Event; +import org.sonar.api.database.model.RuleFailureModel; import org.sonar.api.database.model.Snapshot; import org.sonar.api.design.Dependency; import org.sonar.api.measures.Measure; @@ -47,7 +48,9 @@ public interface PersistenceManager { void saveDependency(Project project, Dependency dependency, Dependency parentDependency); - void saveViolation(Project project, Violation violation); + List loadPreviousViolations(Resource resource); + + void saveOrUpdateViolation(Project project, Violation violation, RuleFailureModel oldModel); void saveLink(Project project, ProjectLink link); diff --git a/sonar-batch/src/main/java/org/sonar/batch/index/ResourcePersister.java b/sonar-batch/src/main/java/org/sonar/batch/index/ResourcePersister.java index 94660c451c4..6a332562cd3 100644 --- a/sonar-batch/src/main/java/org/sonar/batch/index/ResourcePersister.java +++ b/sonar-batch/src/main/java/org/sonar/batch/index/ResourcePersister.java @@ -30,5 +30,7 @@ public interface ResourcePersister { Snapshot saveResource(Project project, Resource resource); + Snapshot getPreviousLastSnapshot(Snapshot snapshot); + void clear(); } 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 caf426f0cfd..f5d2e9661c8 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 @@ -24,10 +24,13 @@ import org.sonar.api.database.DatabaseSession; import org.sonar.api.database.model.RuleFailureModel; import org.sonar.api.database.model.Snapshot; import org.sonar.api.resources.Project; +import org.sonar.api.resources.Resource; import org.sonar.api.rules.Rule; import org.sonar.api.rules.Violation; import org.sonar.api.utils.SonarException; +import java.util.Collections; +import java.util.List; import java.util.Map; public final class ViolationPersister { @@ -41,22 +44,44 @@ public final class ViolationPersister { this.resourcePersister = resourcePersister; } - public void saveViolation(Project project, Violation violation) { - Snapshot snapshot = resourcePersister.saveResource(project, violation.getResource()); - if (snapshot != null) { - session.save(toModel(snapshot, violation)); + public List getPreviousViolations(Resource resource) { + Snapshot snapshot = resourcePersister.getSnapshot(resource); + Snapshot previousLastSnapshot = resourcePersister.getPreviousLastSnapshot(snapshot); + if (previousLastSnapshot == null) { + return Collections.emptyList(); } + return session.getResults(RuleFailureModel.class, "snapshotId", previousLastSnapshot.getId()); } - private RuleFailureModel toModel(Snapshot snapshot, Violation violation) { - RuleFailureModel model = new RuleFailureModel(); - model.setRuleId(getRuleId(violation.getRule())); - model.setPriority(violation.getPriority()); - model.setLine(violation.getLineId()); - model.setMessage(violation.getMessage()); - model.setCost(violation.getCost()); + public void saveOrUpdateViolation(Project project, Violation violation, RuleFailureModel oldModel) { + Snapshot snapshot = resourcePersister.saveResource(project, violation.getResource()); + if (snapshot == null) { + return; // TODO Godin why ? when? + } + RuleFailureModel model; + if (oldModel != null) { + // update + model = session.reattach(RuleFailureModel.class, oldModel.getId()); + model = mergeModel(violation, oldModel); + } else { + // insert + model = createModel(violation); + } model.setSnapshotId(snapshot.getId()); - return model; + session.save(model); + } + + private RuleFailureModel createModel(Violation violation) { + return mergeModel(violation, new RuleFailureModel()); + } + + private RuleFailureModel mergeModel(Violation violation, RuleFailureModel merge) { + merge.setRuleId(getRuleId(violation.getRule())); + merge.setPriority(violation.getPriority()); + merge.setLine(violation.getLineId()); + merge.setMessage(violation.getMessage()); + merge.setCost(violation.getCost()); + return merge; } private Integer getRuleId(Rule rule) { @@ -64,7 +89,8 @@ public final class ViolationPersister { if (ruleId == null) { ruleId = rule.getId(); if (ruleId == null) { - Rule persistedRule = session.getSingleResult(Rule.class, "pluginName", rule.getRepositoryKey(), "key", rule.getKey(), "enabled", true); + Rule persistedRule = session.getSingleResult(Rule.class, + "pluginName", rule.getRepositoryKey(), "key", rule.getKey(), "enabled", true); if (persistedRule == null) { throw new SonarException("Rule not found: " + rule); } diff --git a/sonar-batch/src/main/java/org/sonar/batch/phases/UpdateStatusJob.java b/sonar-batch/src/main/java/org/sonar/batch/phases/UpdateStatusJob.java index 57d5d3b56f5..80c3368816b 100644 --- a/sonar-batch/src/main/java/org/sonar/batch/phases/UpdateStatusJob.java +++ b/sonar-batch/src/main/java/org/sonar/batch/phases/UpdateStatusJob.java @@ -24,6 +24,7 @@ import org.sonar.api.BatchComponent; import org.sonar.api.database.DatabaseSession; import org.sonar.api.database.model.Snapshot; import org.sonar.batch.ServerMetadata; +import org.sonar.batch.index.ResourcePersister; import javax.persistence.Query; @@ -32,27 +33,20 @@ public class UpdateStatusJob implements BatchComponent { private DatabaseSession session; private ServerMetadata server; private Snapshot snapshot; // TODO remove this component + private ResourcePersister resourcePersister; - public UpdateStatusJob(ServerMetadata server, DatabaseSession session, Snapshot snapshot) { + public UpdateStatusJob(ServerMetadata server, DatabaseSession session, ResourcePersister resourcePersister, Snapshot snapshot) { this.session = session; this.server = server; + this.resourcePersister = resourcePersister; this.snapshot = snapshot; } public void execute() { - Snapshot previousLastSnapshot = getPreviousLastSnapshot(snapshot); + Snapshot previousLastSnapshot = resourcePersister.getPreviousLastSnapshot(snapshot); updateFlags(snapshot, previousLastSnapshot); } - private Snapshot getPreviousLastSnapshot(Snapshot snapshot) { - Query query = session.createQuery( - "SELECT s FROM " + Snapshot.class.getSimpleName() + " s " + - "WHERE s.last=true AND s.resourceId=:resourceId"); - query.setParameter("resourceId", snapshot.getResourceId()); - return session.getSingleResult(query, null); - } - - private void updateFlags(Snapshot rootSnapshot, Snapshot previousLastSnapshot) { if (previousLastSnapshot != null && previousLastSnapshot.getCreatedAt().before(rootSnapshot.getCreatedAt())) { setFlags(previousLastSnapshot, false, null); diff --git a/sonar-batch/src/test/java/org/sonar/batch/index/ViolationPersisterTest.java b/sonar-batch/src/test/java/org/sonar/batch/index/ViolationPersisterTest.java index 307295c2e29..0c6b898813d 100644 --- a/sonar-batch/src/test/java/org/sonar/batch/index/ViolationPersisterTest.java +++ b/sonar-batch/src/test/java/org/sonar/batch/index/ViolationPersisterTest.java @@ -21,6 +21,7 @@ package org.sonar.batch.index; import org.junit.Before; import org.junit.Test; +import org.sonar.api.database.model.RuleFailureModel; import org.sonar.api.database.model.Snapshot; import org.sonar.api.resources.JavaFile; import org.sonar.api.resources.Project; @@ -29,6 +30,11 @@ import org.sonar.api.rules.RulePriority; import org.sonar.api.rules.Violation; import org.sonar.jpa.test.AbstractDbUnitTestCase; +import java.util.List; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.greaterThan; +import static org.hamcrest.Matchers.notNullValue; import static org.mockito.Matchers.anyObject; import static org.mockito.Matchers.eq; import static org.mockito.Mockito.mock; @@ -47,9 +53,19 @@ public class ViolationPersisterTest extends AbstractDbUnitTestCase { Snapshot snapshot = getSession().getSingleResult(Snapshot.class, "id", 1000); ResourcePersister resourcePersister = mock(ResourcePersister.class); when(resourcePersister.saveResource((Project) anyObject(), eq(javaFile))).thenReturn(snapshot); + when(resourcePersister.getPreviousLastSnapshot(snapshot)).thenReturn(snapshot); + when(resourcePersister.getSnapshot(javaFile)).thenReturn(snapshot); violationPersister = new ViolationPersister(getSession(), resourcePersister); } + @Test + public void shouldLoadViolations() { + List models = violationPersister.getPreviousViolations(javaFile); + + assertThat(models, notNullValue()); + assertThat(models.size(), greaterThan(0)); + } + @Test public void shouldSaveViolations() { Violation violation1a = Violation.create(rule1, javaFile) @@ -60,10 +76,21 @@ public class ViolationPersisterTest extends AbstractDbUnitTestCase { Violation violation2 = Violation.create(rule2, javaFile) .setPriority(RulePriority.MINOR); - violationPersister.saveViolation(new Project("project"), violation1a); - violationPersister.saveViolation(new Project("project"), violation1b); - violationPersister.saveViolation(new Project("project"), violation2); + violationPersister.saveOrUpdateViolation(new Project("project"), violation1a, null); + violationPersister.saveOrUpdateViolation(new Project("project"), violation1b, null); + violationPersister.saveOrUpdateViolation(new Project("project"), violation2, null); + + checkTables("shouldInsertViolations", "rule_failures"); + } + + @Test + public void shouldUpdateViolation() { + Violation violation = Violation.create(rule1, javaFile) + .setPriority(RulePriority.CRITICAL).setLineId(20).setCost(55.6); + RuleFailureModel oldModel = violationPersister.getPreviousViolations(javaFile).iterator().next(); + + violationPersister.saveOrUpdateViolation(new Project("project"), violation, oldModel); - checkTables("shouldSaveViolations", "rule_failures"); + checkTables("shouldUpdateViolation", "rule_failures"); } } diff --git a/sonar-batch/src/test/java/org/sonar/batch/phases/UpdateStatusJobTest.java b/sonar-batch/src/test/java/org/sonar/batch/phases/UpdateStatusJobTest.java index b83c32eafb0..e6f1e78d657 100644 --- a/sonar-batch/src/test/java/org/sonar/batch/phases/UpdateStatusJobTest.java +++ b/sonar-batch/src/test/java/org/sonar/batch/phases/UpdateStatusJobTest.java @@ -20,8 +20,10 @@ package org.sonar.batch.phases; import org.junit.Test; +import org.sonar.api.database.DatabaseSession; import org.sonar.api.database.model.Snapshot; import org.sonar.batch.ServerMetadata; +import org.sonar.batch.index.DefaultResourcePersister; import org.sonar.jpa.test.AbstractDbUnitTestCase; import javax.persistence.Query; @@ -47,15 +49,14 @@ public class UpdateStatusJobTest extends AbstractDbUnitTestCase { private void assertAnalysis(int snapshotId, String fixture) { setupData("sharedFixture", fixture); - - UpdateStatusJob sensor = new UpdateStatusJob(mock(ServerMetadata.class), getSession(), loadSnapshot(snapshotId)); + DatabaseSession session = getSession(); + UpdateStatusJob sensor = new UpdateStatusJob(mock(ServerMetadata.class), session, new DefaultResourcePersister(session), loadSnapshot(snapshotId)); sensor.execute(); getSession().stop(); checkTables(fixture, "snapshots"); } - private Snapshot loadSnapshot(int id) { Query query = getSession().createQuery("SELECT s FROM Snapshot s WHERE s.id=:id"); query.setParameter("id", id); diff --git a/sonar-batch/src/test/resources/org/sonar/batch/index/ViolationPersisterTest/shared.xml b/sonar-batch/src/test/resources/org/sonar/batch/index/ViolationPersisterTest/shared.xml index b9678a48fcc..0d7a84ff870 100644 --- a/sonar-batch/src/test/resources/org/sonar/batch/index/ViolationPersisterTest/shared.xml +++ b/sonar-batch/src/test/resources/org/sonar/batch/index/ViolationPersisterTest/shared.xml @@ -19,4 +19,5 @@ scope="FIL" qualifier="CLA" created_at="2008-11-01 13:58:00.00" version="[null]" path="" status="U" islast="false" depth="3" /> + \ No newline at end of file diff --git a/sonar-batch/src/test/resources/org/sonar/batch/index/ViolationPersisterTest/shouldSaveViolations-result.xml b/sonar-batch/src/test/resources/org/sonar/batch/index/ViolationPersisterTest/shouldInsertViolations-result.xml similarity index 89% rename from sonar-batch/src/test/resources/org/sonar/batch/index/ViolationPersisterTest/shouldSaveViolations-result.xml rename to sonar-batch/src/test/resources/org/sonar/batch/index/ViolationPersisterTest/shouldInsertViolations-result.xml index 1e78b98346e..c28d5fd6403 100644 --- a/sonar-batch/src/test/resources/org/sonar/batch/index/ViolationPersisterTest/shouldSaveViolations-result.xml +++ b/sonar-batch/src/test/resources/org/sonar/batch/index/ViolationPersisterTest/shouldInsertViolations-result.xml @@ -18,7 +18,8 @@ scope="FIL" qualifier="CLA" created_at="2008-11-01 13:58:00.00" version="[null]" path="" status="U" islast="false" depth="3" /> - - - + + + + \ No newline at end of file diff --git a/sonar-batch/src/test/resources/org/sonar/batch/index/ViolationPersisterTest/shouldUpdateViolation-result.xml b/sonar-batch/src/test/resources/org/sonar/batch/index/ViolationPersisterTest/shouldUpdateViolation-result.xml new file mode 100644 index 00000000000..150433b43af --- /dev/null +++ b/sonar-batch/src/test/resources/org/sonar/batch/index/ViolationPersisterTest/shouldUpdateViolation-result.xml @@ -0,0 +1,22 @@ + + + + + + + + + + + + + + \ No newline at end of file -- 2.39.5