SONAR-14024 fix not purging some pull requests

diff --git a/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/component/BranchPersisterImpl.java b/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/component/BranchPersisterImpl.java
index 1af15d840dc7d05327447b97a77a0d78bab5f021..479626c6fc18d71bc3f9ea107df9b2b881b9c1d6 100644 (file)
--- a/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/component/BranchPersisterImpl.java
+++ b/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/component/BranchPersisterImpl.java
@@ -70,6 +70,10 @@ public class BranchPersisterImpl implements BranchPersister {
       return true;
     }
 
+    if (BranchType.PULL_REQUEST.equals(analysisMetadataHolder.getBranch().getType())) {
+      return false;
+    }
+
     List<String> excludeFromPurgeEntries = asList(ofNullable(configuration.getStringArray(BRANCHES_TO_KEEP_WHEN_INACTIVE)).orElse(new String[0]));
     return excludeFromPurgeEntries.stream()
       .map(Pattern::compile)

diff --git a/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/component/BranchPersisterImplTest.java b/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/component/BranchPersisterImplTest.java
index 4359d0d89504a927ca61dede5c6c62fb14c1e5ba..c5328881e263257856daad25399c6d17f417f851 100644 (file)
--- a/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/component/BranchPersisterImplTest.java
+++ b/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/component/BranchPersisterImplTest.java
@@ -24,6 +24,7 @@ import com.tngtech.java.junit.dataprovider.DataProviderRunner;
 import com.tngtech.java.junit.dataprovider.UseDataProvider;
 import java.util.Optional;
 import javax.annotation.Nullable;
+import org.assertj.core.api.ThrowableAssert;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.ExpectedException;
@@ -32,6 +33,7 @@ import org.sonar.api.config.Configuration;
 import org.sonar.api.utils.System2;
 import org.sonar.ce.task.projectanalysis.analysis.AnalysisMetadataHolderRule;
 import org.sonar.ce.task.projectanalysis.analysis.Branch;
+import org.sonar.db.DbSession;
 import org.sonar.db.DbTester;
 import org.sonar.db.component.BranchDto;
 import org.sonar.db.component.BranchType;
@@ -42,6 +44,7 @@ import org.sonar.server.project.Project;
 
 import static org.apache.commons.lang.RandomStringUtils.randomAlphabetic;
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 import static org.sonar.ce.task.projectanalysis.component.Component.Type.PROJECT;
@@ -52,8 +55,9 @@ import static org.sonar.db.component.BranchType.PULL_REQUEST;
 
 @RunWith(DataProviderRunner.class)
 public class BranchPersisterImplTest {
-  private final static Component MAIN = builder(PROJECT, 1).setUuid("PROJECT_UUID").setKey("PROJECT_KEY").build();
-  private final static Component BRANCH1 = builder(PROJECT, 1).setUuid("BRANCH_UUID").setKey("BRANCH_KEY").build();
+  private final static Component MAIN = builder(PROJECT, 1, "PROJECT_KEY").setUuid("PROJECT_UUID").build();
+  private final static Component BRANCH1 = builder(PROJECT, 2, "BRANCH_KEY").setUuid("BRANCH_UUID").build();
+  private final static Component PR1 = builder(PROJECT, 3, "develop").setUuid("PR_UUID").build();
 
   @Rule
   public AnalysisMetadataHolderRule analysisMetadataHolder = new AnalysisMetadataHolderRule();
@@ -61,41 +65,35 @@ public class BranchPersisterImplTest {
   public DbTester dbTester = DbTester.create(System2.INSTANCE);
   @Rule
   public TreeRootHolderRule treeRootHolder = new TreeRootHolderRule();
-  @Rule
-  public ExpectedException exception = ExpectedException.none();
-
-  private Configuration configuration = mock(Configuration.class);
 
-  private BranchPersister underTest = new BranchPersisterImpl(dbTester.getDbClient(), treeRootHolder, analysisMetadataHolder, configuration);
+  private final Configuration configuration = mock(Configuration.class);
+  private final BranchPersister underTest = new BranchPersisterImpl(dbTester.getDbClient(), treeRootHolder, analysisMetadataHolder, configuration);
 
   @Test
   public void persist_fails_with_ISE_if_no_component_for_main_branches() {
     analysisMetadataHolder.setBranch(createBranch(BRANCH, true, "master"));
     treeRootHolder.setRoot(MAIN);
+    DbSession dbSession = dbTester.getSession();
 
-    expectMissingComponentISE();
-
-    underTest.persist(dbTester.getSession());
+    expectMissingComponentISE(() -> underTest.persist(dbSession));
   }
 
   @Test
   public void persist_fails_with_ISE_if_no_component_for_branches() {
     analysisMetadataHolder.setBranch(createBranch(BRANCH, false, "foo"));
     treeRootHolder.setRoot(BRANCH1);
+    DbSession dbSession = dbTester.getSession();
 
-    expectMissingComponentISE();
-
-    underTest.persist(dbTester.getSession());
+    expectMissingComponentISE(() -> underTest.persist(dbSession));
   }
 
   @Test
   public void persist_fails_with_ISE_if_no_component_for_pull_request() {
     analysisMetadataHolder.setBranch(createBranch(BranchType.PULL_REQUEST, false, "12"));
     treeRootHolder.setRoot(BRANCH1);
+    DbSession dbSession = dbTester.getSession();
 
-    expectMissingComponentISE();
-
-    underTest.persist(dbTester.getSession());
+    expectMissingComponentISE(() -> underTest.persist(dbSession));
   }
 
   @Test
@@ -137,6 +135,7 @@ public class BranchPersisterImplTest {
     underTest.persist(dbTester.getSession());
 
     Optional<BranchDto> branchDto = dbTester.getDbClient().branchDao().selectByUuid(dbTester.getSession(), MAIN.getUuid());
+    assertThat(branchDto).isPresent();
     assertThat(branchDto.get().isExcludeFromPurge()).isTrue();
   }
 
@@ -154,9 +153,34 @@ public class BranchPersisterImplTest {
     underTest.persist(dbTester.getSession());
 
     Optional<BranchDto> branchDto = dbTester.getDbClient().branchDao().selectByUuid(dbTester.getSession(), BRANCH1.getUuid());
+    assertThat(branchDto).isPresent();
     assertThat(branchDto.get().isExcludeFromPurge()).isTrue();
   }
 
+  @Test
+  public void pull_request_is_never_excluded_from_branch_purge_even_if_its_source_branch_name_matches_sonar_dbcleaner_keepFromPurge_property() {
+    when(configuration.getStringArray(BRANCHES_TO_KEEP_WHEN_INACTIVE)).thenReturn(new String[] {"develop"});
+
+    analysisMetadataHolder.setBranch(createPullRequest(PR1.getKey(), MAIN.getUuid()));
+    analysisMetadataHolder.setPullRequestKey(PR1.getKey());
+    treeRootHolder.setRoot(PR1);
+    ComponentDto mainComponent = dbTester.components().insertPublicProject(p -> p.setDbKey(MAIN.getDbKey()).setUuid(MAIN.getUuid()));
+    ComponentDto component = ComponentTesting.newBranchComponent(mainComponent, new BranchDto()
+      .setUuid(PR1.getUuid())
+      .setKey(PR1.getKey())
+      .setProjectUuid(MAIN.getUuid())
+      .setBranchType(PULL_REQUEST)
+      .setMergeBranchUuid(MAIN.getUuid()));
+    dbTester.getDbClient().componentDao().insert(dbTester.getSession(), component);
+    dbTester.commit();
+
+    underTest.persist(dbTester.getSession());
+
+    Optional<BranchDto> branchDto = dbTester.getDbClient().branchDao().selectByUuid(dbTester.getSession(), PR1.getUuid());
+    assertThat(branchDto).isPresent();
+    assertThat(branchDto.get().isExcludeFromPurge()).isFalse();
+  }
+
   @Test
   public void non_main_branch_is_included_in_branch_purge_if_branch_name_does_not_match_sonar_dbcleaner_keepFromPurge_property() {
     when(configuration.getStringArray(BRANCHES_TO_KEEP_WHEN_INACTIVE)).thenReturn(new String[] {"foobar-.*"});
@@ -171,6 +195,7 @@ public class BranchPersisterImplTest {
     underTest.persist(dbTester.getSession());
 
     Optional<BranchDto> branchDto = dbTester.getDbClient().branchDao().selectByUuid(dbTester.getSession(), BRANCH1.getUuid());
+    assertThat(branchDto).isPresent();
     assertThat(branchDto.get().isExcludeFromPurge()).isFalse();
   }
 
@@ -219,6 +244,12 @@ public class BranchPersisterImplTest {
     return createBranch(type, isMain, name, null);
   }
 
+  private static Branch createPullRequest(String key, String mergeBranchUuid) {
+    Branch branch = createBranch(PULL_REQUEST, false, key, mergeBranchUuid);
+    when(branch.getPullRequestKey()).thenReturn(key);
+    return branch;
+  }
+
   private static Branch createBranch(BranchType type, boolean isMain, String name, @Nullable String mergeBranchUuid) {
     Branch branch = mock(Branch.class);
     when(branch.getType()).thenReturn(type);
@@ -229,8 +260,9 @@ public class BranchPersisterImplTest {
     return branch;
   }
 
-  private void expectMissingComponentISE() {
-    exception.expect(IllegalStateException.class);
-    exception.expectMessage("Component has been deleted by end-user during analysis");
+  private void expectMissingComponentISE(ThrowableAssert.ThrowingCallable callable) {
+    assertThatThrownBy(callable)
+      .isInstanceOf(IllegalStateException.class)
+      .hasMessage("Component has been deleted by end-user during analysis");
   }
 }

diff --git a/server/sonar-ce-task-projectanalysis/src/testFixtures/java/org/sonar/ce/task/projectanalysis/component/ReportComponent.java b/server/sonar-ce-task-projectanalysis/src/testFixtures/java/org/sonar/ce/task/projectanalysis/component/ReportComponent.java
index aa29c2a7d4beb7160cece2d904ae0b12f57a2d71..a7b43f91388266e930e83d6ad102b96725d5308d 100644 (file)
--- a/server/sonar-ce-task-projectanalysis/src/testFixtures/java/org/sonar/ce/task/projectanalysis/component/ReportComponent.java
+++ b/server/sonar-ce-task-projectanalysis/src/testFixtures/java/org/sonar/ce/task/projectanalysis/component/ReportComponent.java
@@ -191,11 +191,14 @@ public class ReportComponent implements Component {
       '}';
   }
 
-  public static Builder builder(Type type, int ref) {
-    String key = "key_" + ref;
+  public static Builder builder(Type type, int ref, String key) {
     return new Builder(type, ref).setKey(key).setPublicKey(key).setUuid("uuid_" + ref).setName("name_" + ref);
   }
 
+  public static Builder builder(Type type, int ref) {
+    return builder(type, ref, "key_" + ref);
+  }
+
   public static final class Builder {
     private final Type type;
     private final int ref;

diff --git a/server/sonar-db-dao/src/main/resources/org/sonar/db/purge/PurgeMapper.xml b/server/sonar-db-dao/src/main/resources/org/sonar/db/purge/PurgeMapper.xml
index 828a957dae3848975b62a482e59192c315c9dbdd..e2e04898f386482fde967067cbc1b1f95d31e420 100644 (file)
--- a/server/sonar-db-dao/src/main/resources/org/sonar/db/purge/PurgeMapper.xml
+++ b/server/sonar-db-dao/src/main/resources/org/sonar/db/purge/PurgeMapper.xml
@@ -57,7 +57,7 @@
         and s.islast=${_true}
     where
         pb.project_uuid=#{projectUuid,jdbcType=VARCHAR}
-        and pb.exclude_from_purge = ${_false}
+        and (pb.branch_type = 'PULL_REQUEST' or pb.exclude_from_purge = ${_false})
         and (s.created_at is null or s.created_at < #{toDate})
 
   </select>

diff --git a/server/sonar-db-dao/src/test/java/org/sonar/db/purge/PurgeDaoTest.java b/server/sonar-db-dao/src/test/java/org/sonar/db/purge/PurgeDaoTest.java
index db459e897fdff2f7f4b236eaa11c7d581fd210ae..1c94f954b0c420ceb967da5d098ef28cf990ec2f 100644 (file)
--- a/server/sonar-db-dao/src/test/java/org/sonar/db/purge/PurgeDaoTest.java
+++ b/server/sonar-db-dao/src/test/java/org/sonar/db/purge/PurgeDaoTest.java
@@ -143,27 +143,36 @@ public class PurgeDaoTest {
     ComponentDto project = db.components().insertPublicProject();
     ComponentDto branch1 = db.components().insertProjectBranch(project);
     ComponentDto branch2 = db.components().insertProjectBranch(project, b -> b.setBranchType(BranchType.BRANCH));
+    ComponentDto pr1 = db.components().insertProjectBranch(project, b -> b.setBranchType(BranchType.PULL_REQUEST));
+
     db.components().insertSnapshot(branch1);
     db.components().insertSnapshot(branch2);
+    db.components().insertSnapshot(pr1);
 
     // branch with other components and issues, last analysed 31 days ago
-    ComponentDto branch3 = db.components().insertProjectBranch(project, b -> b.setBranchType(BranchType.BRANCH));
-    db.components().insertSnapshot(branch3, dto -> dto.setCreatedAt(DateUtils.addDays(new Date(), -31).getTime()));
-
-    ComponentDto module = db.components().insertComponent(newModuleDto(branch3));
-    ComponentDto subModule = db.components().insertComponent(newModuleDto(module));
-    ComponentDto file = db.components().insertComponent(newFileDto(subModule));
-    db.issues().insert(rule, branch3, file);
-    db.issues().insert(rule, branch3, subModule);
-    db.issues().insert(rule, branch3, module);
+    ComponentDto branch3 = db.components().insertProjectBranch(project, b -> b.setBranchType(BranchType.BRANCH).setExcludeFromPurge(false));
+    addComponentsSnapshotsAndIssuesToBranch(branch3, rule, 31);
 
     // branch with no analysis
     ComponentDto branch4 = db.components().insertProjectBranch(project, b -> b.setBranchType(BranchType.BRANCH));
 
+    // branch last analysed 31 days ago but protected from purge
+    ComponentDto branch5 = db.components().insertProjectBranch(project, b -> b.setBranchType(BranchType.BRANCH).setExcludeFromPurge(true));
+    db.components().insertSnapshot(branch5, dto -> dto.setCreatedAt(DateUtils.addDays(new Date(), -31).getTime()));
+
+    // pull request last analysed 100 days ago
+    ComponentDto pr2 = db.components().insertProjectBranch(project, b -> b.setBranchType(BranchType.PULL_REQUEST).setExcludeFromPurge(false));
+    addComponentsSnapshotsAndIssuesToBranch(pr2, rule, 100);
+
+    // pull request last analysed 100 days ago but marked as "excluded from purge" which should not work for pull requests
+    ComponentDto pr3 = db.components().insertProjectBranch(project, b -> b.setBranchType(BranchType.PULL_REQUEST).setExcludeFromPurge(true));
+    addComponentsSnapshotsAndIssuesToBranch(pr3, rule, 100);
+
     underTest.purge(dbSession, newConfigurationWith30Days(System2.INSTANCE, project.uuid(), project.uuid()), PurgeListener.EMPTY, new PurgeProfiler());
     dbSession.commit();
 
-    assertThat(uuidsIn("components")).containsOnly(project.uuid(), branch1.uuid(), branch2.uuid());
+    assertThat(uuidsIn("components")).containsOnly(
+      project.uuid(), branch1.uuid(), branch2.uuid(), branch5.uuid(), pr1.uuid());
     assertThat(uuidsIn("projects")).containsOnly(project.uuid());
   }
 
@@ -1773,4 +1782,14 @@ public class PurgeDaoTest {
       .map(t -> (String) t.get("UUID"));
   }
 
+  private void addComponentsSnapshotsAndIssuesToBranch(ComponentDto branch, RuleDefinitionDto rule, int branchAge) {
+    db.components().insertSnapshot(branch, dto -> dto.setCreatedAt(DateUtils.addDays(new Date(), -branchAge).getTime()));
+    ComponentDto module = db.components().insertComponent(newModuleDto(branch));
+    ComponentDto subModule = db.components().insertComponent(newModuleDto(module));
+    ComponentDto file = db.components().insertComponent(newFileDto(subModule));
+    db.issues().insert(rule, branch, file);
+    db.issues().insert(rule, branch, subModule);
+    db.issues().insert(rule, branch, module);
+  }
+
 }