From 94db5278d13749bc20bc98e9cd6c9e44d3798249 Mon Sep 17 00:00:00 2001 From: Michal Duda Date: Wed, 2 Dec 2020 13:02:05 +0100 Subject: [PATCH] SONAR-14024 fix not purging some pull requests --- .../component/BranchPersisterImpl.java | 4 ++ .../component/BranchPersisterImplTest.java | 70 ++++++++++++++----- .../component/ReportComponent.java | 7 +- .../org/sonar/db/purge/PurgeMapper.xml | 2 +- .../java/org/sonar/db/purge/PurgeDaoTest.java | 39 ++++++++--- 5 files changed, 90 insertions(+), 32 deletions(-) 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 1af15d840dc..479626c6fc1 100644 --- 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 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 4359d0d8950..c5328881e26 100644 --- 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 = 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 = 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 = 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 = 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 aa29c2a7d4b..a7b43f91388 100644 --- 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 828a957dae3..e2e04898f38 100644 --- 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}) 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 db459e897fd..1c94f954b0c 100644 --- 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); + } + } -- 2.39.5