Increase code coverage + fix SQL pitfall

3 files changed, 86 insertions, 20 deletions

diff --git a/plugins/sonar-core-plugin/src/main/java/org/sonar/plugins/core/sensors/CloseReviewsDecorator.java b/plugins/sonar-core-plugin/src/main/java/org/sonar/plugins/core/sensors/CloseReviewsDecorator.java
index bec450bc34b..6eae4132f76 100644
--- a/plugins/sonar-core-plugin/src/main/java/org/sonar/plugins/core/sensors/CloseReviewsDecorator.java
+++ b/plugins/sonar-core-plugin/src/main/java/org/sonar/plugins/core/sensors/CloseReviewsDecorator.java
@@ -45,9 +45,17 @@ import java.util.List;
 @DependedUpon(CloseReviewsDecorator.REVIEW_LIFECYCLE_BARRIER)
 public class CloseReviewsDecorator implements Decorator {
 
-  private static final Logger LOG = LoggerFactory.getLogger(CloseReviewsDecorator.class);
   public static final String REVIEW_LIFECYCLE_BARRIER = "REVIEW_LIFECYCLE_BARRIER";
 
+  private static final String PROJECT_ID = "projectId";
+  private static final String PROJECT_SNAPSHOT_ID = "projectSnapshotId";
+  private static final String SNAPSHOT_ID = "snapshotId";
+  private static final String RESOURCE_ID = "resourceId";
+  private static final String IS_LAST = "isLast";
+  private static final String MANUAL_VIOLATION = "manualViolation";
+  private static final String AUTOMATIC_VIOLATION = "automaticViolation";
+  private static final Logger LOG = LoggerFactory.getLogger(CloseReviewsDecorator.class);
+
   private Project project;
   private ResourcePersister resourcePersister;
   private DatabaseSession databaseSession;
@@ -67,6 +75,7 @@ public class CloseReviewsDecorator implements Decorator {
     return project.isLatestAnalysis();
   }
 
+  @SuppressWarnings("rawtypes")
   public void decorate(Resource resource, DecoratorContext context) {
     Snapshot currentSnapshot = resourcePersister.getSnapshot(resource);
     if (currentSnapshot != null) {
@@ -87,19 +96,22 @@ public class CloseReviewsDecorator implements Decorator {
   /**
    * Close reviews for which violations have been fixed.
    */
-  protected int closeReviewsOnResolvedViolations(Resource resource, int resourceId, int snapshotId) {
-    String conditions = " WHERE resource_id=" + resourceId + " AND "
+  @SuppressWarnings({"unchecked"})
+  protected int closeReviewsOnResolvedViolations(Resource<?> resource, int resourceId, int snapshotId) {
+    String conditions = " WHERE resource_id=:" + RESOURCE_ID + " AND "
       + "( "
       + "  ( "
-      + "    manual_violation=:automaticViolation AND status!='CLOSED' AND "
-      + "    rule_failure_permanent_id NOT IN (SELECT permanent_id FROM rule_failures WHERE snapshot_id=" + snapshotId + " AND permanent_id IS NOT NULL)"
+      + "    manual_violation=:" + AUTOMATIC_VIOLATION + " AND status!='CLOSED' AND "
+      + "    rule_failure_permanent_id NOT IN (SELECT permanent_id FROM rule_failures WHERE snapshot_id=:" + SNAPSHOT_ID + " AND permanent_id IS NOT NULL)"
       + "  )"
       + "  OR "
-      + "  (manual_violation=:manualViolation AND status='RESOLVED')"
+      + "  (manual_violation=:" + MANUAL_VIOLATION + " AND status='RESOLVED')"
       + ")";
     List<Review> reviews = databaseSession.getEntityManager().createNativeQuery("SELECT * FROM reviews " + conditions, Review.class)
-      .setParameter("automaticViolation", false)
-      .setParameter("manualViolation", true)
+      .setParameter(RESOURCE_ID, resourceId)
+      .setParameter(SNAPSHOT_ID, snapshotId)
+      .setParameter(AUTOMATIC_VIOLATION, false)
+      .setParameter(MANUAL_VIOLATION, true)
       .getResultList();
 
     for (Review review : reviews) {
@@ -107,8 +119,10 @@ public class CloseReviewsDecorator implements Decorator {
     }
 
     int rowUpdated = databaseSession.createNativeQuery("UPDATE reviews SET status='CLOSED', updated_at=CURRENT_TIMESTAMP" + conditions)
-      .setParameter("automaticViolation", false)
-      .setParameter("manualViolation", true)
+      .setParameter(RESOURCE_ID, resourceId)
+      .setParameter(SNAPSHOT_ID, snapshotId)
+      .setParameter(AUTOMATIC_VIOLATION, false)
+      .setParameter(MANUAL_VIOLATION, true)
       .executeUpdate();
     if (rowUpdated > 0) {
       LOG.debug("- {} reviews closed on #{}", rowUpdated, resourceId);
@@ -120,17 +134,20 @@ public class CloseReviewsDecorator implements Decorator {
    * Reopen reviews that had been set to resolved but for which the violation is still here.
    * Manual violations are ignored.
    */
-  protected int reopenReviewsOnUnresolvedViolations(Resource resource, int resourceId) {
-    String conditions = " WHERE status='RESOLVED' AND resolution<>'FALSE-POSITIVE' AND manual_violation=:manualViolation AND resource_id=" + resourceId;
+  @SuppressWarnings({"unchecked"})
+  protected int reopenReviewsOnUnresolvedViolations(Resource<?> resource, int resourceId) {
+    String conditions = " WHERE status='RESOLVED' AND resolution<>'FALSE-POSITIVE' AND manual_violation=:" + MANUAL_VIOLATION + " AND resource_id=:" + RESOURCE_ID;
     List<Review> reviews = databaseSession.getEntityManager().createNativeQuery("SELECT * FROM reviews " + conditions, Review.class)
-      .setParameter("manualViolation", false)
+      .setParameter(RESOURCE_ID, resourceId)
+      .setParameter(MANUAL_VIOLATION, false)
       .getResultList();
     for (Review review : reviews) {
       notifyReopened(resource, review);
     }
 
     int rowUpdated = databaseSession.createNativeQuery("UPDATE reviews SET status='REOPENED', resolution=NULL, updated_at=CURRENT_TIMESTAMP" + conditions)
-      .setParameter("manualViolation", false)
+      .setParameter(RESOURCE_ID, resourceId)
+      .setParameter(MANUAL_VIOLATION, false)
       .executeUpdate();
     if (rowUpdated > 0) {
       LOG.debug("- {} reviews reopened on #{}", rowUpdated, resourceId);
@@ -141,19 +158,24 @@ public class CloseReviewsDecorator implements Decorator {
   /**
    * Close reviews that relate to resources that have been deleted or renamed.
    */
+  @SuppressWarnings("unchecked")
   protected int closeReviewsForDeletedResources(int projectId, int projectSnapshotId) {
-    String conditions = " WHERE status!='CLOSED' AND project_id=" + projectId
-      + " AND resource_id IN ( SELECT prev.project_id FROM snapshots prev  WHERE prev.root_project_id=" + projectId
-      + " AND prev.islast=? AND NOT EXISTS ( SELECT cur.id FROM snapshots cur WHERE cur.root_snapshot_id=" + projectSnapshotId
-      + " AND cur.created_at > prev.created_at AND cur.root_project_id=" + projectId + " AND cur.project_id=prev.project_id ) )";
+    String conditions = " WHERE status!='CLOSED' AND project_id=:" + PROJECT_ID
+      + " AND resource_id IN ( SELECT prev.project_id FROM snapshots prev  WHERE prev.root_project_id=:" + PROJECT_ID 
+      + " AND prev.islast=:" + IS_LAST + " AND NOT EXISTS ( SELECT cur.id FROM snapshots cur WHERE cur.root_snapshot_id=:" + PROJECT_SNAPSHOT_ID
+      + " AND cur.created_at > prev.created_at AND cur.root_project_id=:" + PROJECT_ID + " AND cur.project_id=prev.project_id ) )";
     List<Review> reviews = databaseSession.getEntityManager().createNativeQuery("SELECT * FROM reviews " + conditions, Review.class)
-      .setParameter(1, Boolean.TRUE)
+      .setParameter(PROJECT_ID, projectId)
+      .setParameter(PROJECT_SNAPSHOT_ID, projectSnapshotId)
+      .setParameter(IS_LAST, Boolean.TRUE)
       .getResultList();
     for (Review review : reviews) {
       notifyClosed(null, review);
     }
     int rowUpdated = databaseSession.createNativeQuery("UPDATE reviews SET status='CLOSED', updated_at=CURRENT_TIMESTAMP" + conditions)
-      .setParameter(1, Boolean.TRUE)
+      .setParameter(PROJECT_ID, projectId)
+      .setParameter(PROJECT_SNAPSHOT_ID, projectSnapshotId)
+      .setParameter(IS_LAST, Boolean.TRUE)
       .executeUpdate();
     LOG.debug("- {} reviews set to 'closed' on project #{}", rowUpdated, projectSnapshotId);
     return rowUpdated;
diff --git a/plugins/sonar-core-plugin/src/test/java/org/sonar/plugins/core/sensors/CloseReviewsDecoratorTest.java b/plugins/sonar-core-plugin/src/test/java/org/sonar/plugins/core/sensors/CloseReviewsDecoratorTest.java
index 27ec2e039d0..09b748903a5 100644
--- a/plugins/sonar-core-plugin/src/test/java/org/sonar/plugins/core/sensors/CloseReviewsDecoratorTest.java
+++ b/plugins/sonar-core-plugin/src/test/java/org/sonar/plugins/core/sensors/CloseReviewsDecoratorTest.java
@@ -108,4 +108,17 @@ public class CloseReviewsDecoratorTest extends AbstractDbUnitTestCase {
     assertThat(getSession().getSingleResult(Review.class, "id", 2L).getStatus(), is("CLOSED")); // manual violation resolved -> closed
     assertThat(getSession().getSingleResult(Review.class, "id", 3L).getStatus(), is("OPEN"));   // manual violation not changed
   }
+
+  @Test
+  public void shouldCloseReviewCorrespondingToDeletedResource() throws Exception {
+    setupData("shouldCloseReviewCorrespondingToDeletedResource");
+
+    int count = reviewsDecorator.closeReviewsForDeletedResources(111, 11);
+    assertThat(count, is(1));
+
+    verify(notificationManager, times(1)).scheduleForSending(any(Notification.class));
+
+    getSession().commit();
+    assertThat(getSession().getSingleResult(Review.class, "id", 1L).getStatus(), is("CLOSED"));
+  }
 }
diff --git a/plugins/sonar-core-plugin/src/test/resources/org/sonar/plugins/core/sensors/CloseReviewsDecoratorTest/shouldCloseReviewCorrespondingToDeletedResource.xml b/plugins/sonar-core-plugin/src/test/resources/org/sonar/plugins/core/sensors/CloseReviewsDecoratorTest/shouldCloseReviewCorrespondingToDeletedResource.xml
new file mode 100644
index 00000000000..847c740236d
--- /dev/null
+++ b/plugins/sonar-core-plugin/src/test/resources/org/sonar/plugins/core/sensors/CloseReviewsDecoratorTest/shouldCloseReviewCorrespondingToDeletedResource.xml
@@ -0,0 +1,31 @@
+<dataset>
+
+  <snapshots
+    purge_status="[null]"
+    id="11"
+    project_id="555"
+    root_project_id="111"
+    status="P" islast="true"/>
+
+  <!--
+
+  Reviews on automatic violation
+
+  -->
+  <reviews
+    id="1"
+    status="OPEN"
+    rule_failure_permanent_id="1"
+    resolution="[null]"
+    created_at="[null]"
+    updated_at="[null]"
+    project_id="111"
+    resource_line="[null]"
+    severity="MINOR"
+    user_id="[null]"
+    resource_id="555"
+    rule_id="1"
+    manual_violation="false"
+    manual_severity="false"/>
+
+</dataset>
\ No newline at end of file