From 34471db69adf3ca56fe4c17496c6c82ac6036c97 Mon Sep 17 00:00:00 2001 From: =?utf8?q?S=C3=A9bastien=20Lesaint?= Date: Wed, 17 Jan 2018 14:35:48 +0100 Subject: [PATCH] SONAR-10247 do not call webhooks when QG status has not changed --- .../measure/live/LiveMeasureComputerImpl.java | 21 ++-- .../webhook/WebhookQGChangeEventListener.java | 31 +++++- .../WebhookQGChangeEventListenerTest.java | 101 +++++++++++++++--- .../org/sonarqube/tests/Category3Suite.java | 1 + .../sonarqube/tests/webhook/WebhooksTest.java | 21 +++- 5 files changed, 146 insertions(+), 29 deletions(-) diff --git a/server/sonar-server/src/main/java/org/sonar/server/measure/live/LiveMeasureComputerImpl.java b/server/sonar-server/src/main/java/org/sonar/server/measure/live/LiveMeasureComputerImpl.java index 4bfc67be844..9974d3eca3c 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/measure/live/LiveMeasureComputerImpl.java +++ b/server/sonar-server/src/main/java/org/sonar/server/measure/live/LiveMeasureComputerImpl.java @@ -29,7 +29,6 @@ import java.util.Optional; import java.util.Set; import javax.annotation.CheckForNull; import org.sonar.api.config.Configuration; -import org.sonar.api.measures.CoreMetrics; import org.sonar.api.measures.Metric; import org.sonar.api.utils.log.Loggers; import org.sonar.db.DbClient; @@ -53,6 +52,7 @@ import static com.google.common.base.Preconditions.checkState; import static java.util.Collections.emptyList; import static java.util.Collections.singleton; import static java.util.stream.Collectors.groupingBy; +import static org.sonar.api.measures.CoreMetrics.ALERT_STATUS_KEY; import static org.sonar.core.util.stream.MoreCollectors.toArrayList; import static org.sonar.core.util.stream.MoreCollectors.uniqueIndex; @@ -104,10 +104,13 @@ public class LiveMeasureComputerImpl implements LiveMeasureComputer { QualityGate qualityGate = qGateComputer.loadQualityGate(dbSession, organization, project, branch); Collection metricKeys = getKeysOfAllInvolvedMetrics(qualityGate); - Map metricsPerId = dbClient.metricDao().selectByKeys(dbSession, metricKeys).stream() + List metrics = dbClient.metricDao().selectByKeys(dbSession, metricKeys); + Map metricsPerId = metrics.stream() .collect(uniqueIndex(MetricDto::getId)); List componentUuids = components.stream().map(ComponentDto::uuid).collect(toArrayList(components.size())); List dbMeasures = dbClient.liveMeasureDao().selectByComponentUuidsAndMetricIds(dbSession, componentUuids, metricsPerId.keySet()); + // previous status must be load now as MeasureMatrix mutate the LiveMeasureDto which are passed to it + Metric.Level previousStatus = loadPreviousStatus(metrics, dbMeasures); Configuration config = projectConfigurationLoader.loadProjectConfiguration(dbSession, project); DebtRatingGrid debtRatingGrid = new DebtRatingGrid(config); @@ -135,15 +138,19 @@ public class LiveMeasureComputerImpl implements LiveMeasureComputer { matrix.getChanged().forEach(m -> dbClient.liveMeasureDao().insertOrUpdate(dbSession, m, null)); projectIndexer.commitAndIndex(dbSession, singleton(project), ProjectIndexer.Cause.MEASURE_CHANGE); - Metric.Level previousStatus = loadPreviousStatus(project, matrix); return Optional.of( new QGChangeEvent(project, branch, lastAnalysis.get(), config, previousStatus, () -> Optional.of(evaluatedQualityGate))); } @CheckForNull - private static Metric.Level loadPreviousStatus(ComponentDto project, MeasureMatrix matrix) { - return matrix.getMeasure(project, CoreMetrics.ALERT_STATUS_KEY) - .map(liveMeasureDto -> liveMeasureDto.getTextValue()) + private static Metric.Level loadPreviousStatus(List metrics, List dbMeasures) { + MetricDto alertStatusMetric = metrics.stream() + .filter(m -> ALERT_STATUS_KEY.equals(m.getKey())) + .findAny() + .orElseThrow(() -> new IllegalStateException(String.format("Metric with key %s is not registered", ALERT_STATUS_KEY))); + return dbMeasures.stream() + .filter(m -> m.getMetricId() == alertStatusMetric.getId()) + .map(LiveMeasureDto::getTextValue) .filter(Objects::nonNull) .map(m -> { try { @@ -154,6 +161,8 @@ public class LiveMeasureComputerImpl implements LiveMeasureComputer { return null; } }) + .filter(Objects::nonNull) + .findAny() .orElse(null); } diff --git a/server/sonar-server/src/main/java/org/sonar/server/webhook/WebhookQGChangeEventListener.java b/server/sonar-server/src/main/java/org/sonar/server/webhook/WebhookQGChangeEventListener.java index 85c1a460e95..f73d05506ff 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/webhook/WebhookQGChangeEventListener.java +++ b/server/sonar-server/src/main/java/org/sonar/server/webhook/WebhookQGChangeEventListener.java @@ -20,15 +20,19 @@ package org.sonar.server.webhook; import java.util.Map; +import java.util.Optional; import java.util.Set; import java.util.stream.Collectors; +import javax.annotation.Nullable; import org.apache.commons.lang.StringUtils; +import org.sonar.api.measures.Metric; import org.sonar.db.DbClient; import org.sonar.db.DbSession; import org.sonar.db.component.AnalysisPropertyDto; import org.sonar.db.component.BranchDto; import org.sonar.db.component.ComponentDto; import org.sonar.db.component.SnapshotDto; +import org.sonar.server.qualitygate.EvaluatedQualityGate; import org.sonar.server.qualitygate.changeevent.QGChangeEvent; import org.sonar.server.qualitygate.changeevent.QGChangeEventListener; import org.sonar.server.webhook.Branch.Type; @@ -49,20 +53,37 @@ public class WebhookQGChangeEventListener implements QGChangeEventListener { if (!webhooks.isEnabled(qualityGateEvent.getProjectConfiguration())) { return; } + Optional evaluatedQualityGate = qualityGateEvent.getQualityGateSupplier().get(); + if (isQGStatusUnchanged(qualityGateEvent, evaluatedQualityGate)) { + return; + } try (DbSession dbSession = dbClient.openSession(false)) { - callWebhook(dbSession, qualityGateEvent); + callWebhook(dbSession, qualityGateEvent, evaluatedQualityGate.orElse(null)); + } + } + + private static boolean isQGStatusUnchanged(QGChangeEvent qualityGateEvent, Optional evaluatedQualityGate) { + Optional previousStatus = qualityGateEvent.getPreviousStatus(); + if (!previousStatus.isPresent() && !evaluatedQualityGate.isPresent()) { + return true; } + + return previousStatus + .map(previousQGStatus -> evaluatedQualityGate + .filter(newQualityGate -> newQualityGate.getStatus() == previousQGStatus) + .isPresent()) + .orElse(false); } - private void callWebhook(DbSession dbSession, QGChangeEvent event) { + private void callWebhook(DbSession dbSession, QGChangeEvent event, @Nullable EvaluatedQualityGate evaluatedQualityGate) { webhooks.sendProjectAnalysisUpdate( event.getProjectConfiguration(), new WebHooks.Analysis(event.getBranch().getUuid(), event.getAnalysis().getUuid(), null), - () -> buildWebHookPayload(dbSession, event)); + () -> buildWebHookPayload(dbSession, event, evaluatedQualityGate)); } - private WebhookPayload buildWebHookPayload(DbSession dbSession, QGChangeEvent event) { + private WebhookPayload buildWebHookPayload(DbSession dbSession, QGChangeEvent event, @Nullable EvaluatedQualityGate evaluatedQualityGate) { ComponentDto project = event.getProject(); BranchDto branch = event.getBranch(); SnapshotDto analysis = event.getAnalysis(); @@ -75,7 +96,7 @@ public class WebhookQGChangeEventListener implements QGChangeEventListener { null, new Analysis(analysis.getUuid(), analysis.getCreatedAt()), new Branch(branch.isMain(), branch.getKey(), Type.valueOf(branch.getBranchType().name())), - event.getQualityGateSupplier().get().orElse(null), + evaluatedQualityGate, null, analysisProperties); return webhookPayloadFactory.create(projectAnalysis); diff --git a/server/sonar-server/src/test/java/org/sonar/server/webhook/WebhookQGChangeEventListenerTest.java b/server/sonar-server/src/test/java/org/sonar/server/webhook/WebhookQGChangeEventListenerTest.java index 747e5c802ff..e93bc8321c5 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/webhook/WebhookQGChangeEventListenerTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/webhook/WebhookQGChangeEventListenerTest.java @@ -19,6 +19,10 @@ */ package org.sonar.server.webhook; +import com.tngtech.java.junit.dataprovider.DataProvider; +import com.tngtech.java.junit.dataprovider.DataProviderRunner; +import com.tngtech.java.junit.dataprovider.UseDataProvider; +import java.util.Arrays; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -29,6 +33,7 @@ import java.util.function.Supplier; import javax.annotation.Nullable; import org.junit.Rule; import org.junit.Test; +import org.junit.runner.RunWith; import org.mockito.ArgumentCaptor; import org.mockito.Matchers; import org.mockito.Mockito; @@ -45,13 +50,13 @@ import org.sonar.db.component.ComponentDto; import org.sonar.db.component.SnapshotDto; import org.sonar.db.organization.OrganizationDto; import org.sonar.server.qualitygate.EvaluatedQualityGate; -import org.sonar.server.qualitygate.QualityGate; -import org.sonar.server.qualitygate.ShortLivingBranchQualityGate; import org.sonar.server.qualitygate.changeevent.QGChangeEvent; import org.sonar.server.qualitygate.changeevent.QGChangeEventListener; -import static java.lang.String.valueOf; +import static java.util.Arrays.stream; import static java.util.Collections.emptySet; +import static java.util.stream.Stream.concat; +import static java.util.stream.Stream.of; import static org.apache.commons.lang.RandomStringUtils.randomAlphanumeric; import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.Matchers.any; @@ -64,12 +69,9 @@ import static org.sonar.db.component.BranchType.LONG; import static org.sonar.db.component.ComponentTesting.newBranchDto; import static org.sonar.db.component.ComponentTesting.newPrivateProjectDto; +@RunWith(DataProviderRunner.class) public class WebhookQGChangeEventListenerTest { - private static final EvaluatedQualityGate EVALUATED_QUALITY_GATE_1 = EvaluatedQualityGate.newBuilder() - .setQualityGate(new QualityGate(valueOf(ShortLivingBranchQualityGate.ID), ShortLivingBranchQualityGate.NAME, emptySet())) - .setStatus(Metric.Level.OK) - .build(); private static final Set CHANGED_ISSUES_ARE_IGNORED = emptySet(); @Rule @@ -77,6 +79,7 @@ public class WebhookQGChangeEventListenerTest { private DbClient dbClient = dbTester.getDbClient(); + private EvaluatedQualityGate newQualityGate = mock(EvaluatedQualityGate.class); private WebHooks webHooks = mock(WebHooks.class); private WebhookPayloadFactory webhookPayloadFactory = mock(WebhookPayloadFactory.class); private DbClient spiedOnDbClient = Mockito.spy(dbClient); @@ -85,11 +88,12 @@ public class WebhookQGChangeEventListenerTest { private WebhookQGChangeEventListener mockedUnderTest = new WebhookQGChangeEventListener(webHooks, webhookPayloadFactory, mockedDbClient); @Test - public void onIssueChanges_has_no_effect_if_no_webhook_is_configured() { + @UseDataProvider("allCombinationsOfStatuses") + public void onIssueChanges_has_no_effect_if_no_webhook_is_configured(Metric.Level previousStatus, Metric.Level newStatus) { Configuration configuration1 = mock(Configuration.class); mockWebhookDisabled(configuration1); - Metric.Level previousStatus = Metric.Level.values()[new Random().nextInt(Metric.Level.values().length)]; - QGChangeEvent qualityGateEvent = new QGChangeEvent(new ComponentDto(), new BranchDto(), new SnapshotDto(), configuration1, previousStatus, Optional::empty); + when(newQualityGate.getStatus()).thenReturn(newStatus); + QGChangeEvent qualityGateEvent = newQGChangeEvent(configuration1, previousStatus, newQualityGate); mockedUnderTest.onIssueChanges(qualityGateEvent, CHANGED_ISSUES_ARE_IGNORED); @@ -97,8 +101,49 @@ public class WebhookQGChangeEventListenerTest { verifyZeroInteractions(webhookPayloadFactory, mockedDbClient); } + @DataProvider + public static Object[][] allCombinationsOfStatuses() { + Metric.Level[] levelsAndNull = concat(of((Metric.Level) null), stream(Metric.Level.values())) + .toArray(Metric.Level[]::new); + Object[][] res = new Object[levelsAndNull.length * levelsAndNull.length][2]; + int i = 0; + for (Metric.Level previousStatus : Arrays.asList(levelsAndNull)) { + for (Metric.Level newStatus : Arrays.asList(levelsAndNull)) { + res[i][0] = previousStatus; + res[i][1] = newStatus; + i++; + } + } + return res; + } + + @Test + public void onIssueChanges_has_no_effect_if_event_has_neither_previousQGStatus_nor_qualityGate() { + Configuration configuration = mock(Configuration.class); + mockWebhookEnabled(configuration); + QGChangeEvent qualityGateEvent = newQGChangeEvent(configuration, null, null); + + underTest.onIssueChanges(qualityGateEvent, CHANGED_ISSUES_ARE_IGNORED); + + verifyZeroInteractions(webhookPayloadFactory, mockedDbClient); + } + + @Test + public void onIssueChanges_has_no_effect_if_event_has_same_status_in_previous_and_new_QG() { + Configuration configuration = mock(Configuration.class); + mockWebhookEnabled(configuration); + Metric.Level previousStatus = randomLevel(); + when(newQualityGate.getStatus()).thenReturn(previousStatus); + QGChangeEvent qualityGateEvent = newQGChangeEvent(configuration, previousStatus, newQualityGate); + + underTest.onIssueChanges(qualityGateEvent, CHANGED_ISSUES_ARE_IGNORED); + + verifyZeroInteractions(webhookPayloadFactory, mockedDbClient); + } + @Test - public void onIssueChanges_calls_webhook_for_changeEvent_with_webhook_enabled() { + @UseDataProvider("newQGorNot") + public void onIssueChanges_calls_webhook_for_changeEvent_with_webhook_enabled(@Nullable EvaluatedQualityGate newQualityGate) { OrganizationDto organization = dbTester.organizations().insert(); ComponentDto project = dbTester.components().insertPublicProject(organization); ComponentAndBranch branch = insertProjectBranch(project, BranchType.SHORT, "foo"); @@ -110,7 +155,7 @@ public class WebhookQGChangeEventListenerTest { properties.put("sonar.analysis.test1", randomAlphanumeric(50)); properties.put("sonar.analysis.test2", randomAlphanumeric(5000)); insertPropertiesFor(analysis.getUuid(), properties); - QGChangeEvent qualityGateEvent = newQGChangeEvent(branch, analysis, configuration, EVALUATED_QUALITY_GATE_1); + QGChangeEvent qualityGateEvent = newQGChangeEvent(branch, analysis, configuration, newQualityGate); underTest.onIssueChanges(qualityGateEvent, CHANGED_ISSUES_ARE_IGNORED); @@ -121,19 +166,20 @@ public class WebhookQGChangeEventListenerTest { null, new Analysis(analysis.getUuid(), analysis.getCreatedAt()), new Branch(false, "foo", Branch.Type.SHORT), - EVALUATED_QUALITY_GATE_1, + newQualityGate, null, properties)); } @Test - public void onIssueChanges_calls_webhook_on_main_branch() { + @UseDataProvider("newQGorNot") + public void onIssueChanges_calls_webhook_on_main_branch(@Nullable EvaluatedQualityGate newQualityGate) { OrganizationDto organization = dbTester.organizations().insert(); ComponentAndBranch mainBranch = insertMainBranch(organization); SnapshotDto analysis = insertAnalysisTask(mainBranch); Configuration configuration = mock(Configuration.class); mockWebhookEnabled(configuration); - QGChangeEvent qualityGateEvent = newQGChangeEvent(mainBranch, analysis, configuration, EVALUATED_QUALITY_GATE_1); + QGChangeEvent qualityGateEvent = newQGChangeEvent(mainBranch, analysis, configuration, newQualityGate); underTest.onIssueChanges(qualityGateEvent, CHANGED_ISSUES_ARE_IGNORED); @@ -164,6 +210,15 @@ public class WebhookQGChangeEventListenerTest { verifyWebhookCalled(longBranch, analysis, configuration); } + @DataProvider + public static Object[][] newQGorNot() { + EvaluatedQualityGate newQualityGate = mock(EvaluatedQualityGate.class); + return new Object[][] { + {null}, + {newQualityGate} + }; + } + private void mockWebhookEnabled(Configuration... configurations) { for (Configuration configuration : configurations) { when(webHooks.isEnabled(configuration)).thenReturn(true); @@ -260,9 +315,23 @@ public class WebhookQGChangeEventListenerTest { } + private static QGChangeEvent newQGChangeEvent(Configuration configuration, @Nullable Metric.Level previousQQStatus, @Nullable EvaluatedQualityGate evaluatedQualityGate) { + return new QGChangeEvent(new ComponentDto(), new BranchDto(), new SnapshotDto(), configuration, previousQQStatus, () -> Optional.ofNullable(evaluatedQualityGate)); + } + private static QGChangeEvent newQGChangeEvent(ComponentAndBranch branch, SnapshotDto analysis, Configuration configuration, @Nullable EvaluatedQualityGate evaluatedQualityGate) { - Metric.Level previousStatus = Metric.Level.values()[new Random().nextInt(Metric.Level.values().length)]; + Metric.Level previousStatus = randomLevel(); + if (evaluatedQualityGate != null) { + Metric.Level otherLevel = stream(Metric.Level.values()) + .filter(s -> s != previousStatus) + .toArray(Metric.Level[]::new)[new Random().nextInt(Metric.Level.values().length - 1)]; + when(evaluatedQualityGate.getStatus()).thenReturn(otherLevel); + } return new QGChangeEvent(branch.component, branch.branch, analysis, configuration, previousStatus, () -> Optional.ofNullable(evaluatedQualityGate)); } + private static Metric.Level randomLevel() { + return Metric.Level.values()[new Random().nextInt(Metric.Level.values().length)]; + } + } diff --git a/tests/src/test/java/org/sonarqube/tests/Category3Suite.java b/tests/src/test/java/org/sonarqube/tests/Category3Suite.java index 0f066ca1de4..60fbc385ab8 100644 --- a/tests/src/test/java/org/sonarqube/tests/Category3Suite.java +++ b/tests/src/test/java/org/sonarqube/tests/Category3Suite.java @@ -92,6 +92,7 @@ public class Category3Suite { // reduce memory for Elasticsearch to 128M .setServerProperty("sonar.search.javaOpts", "-Xms128m -Xmx128m") +// .setServerProperty("sonar.web.javaAdditionalOpts", "-agentlib:jdwp=transport=dt_socket,server=y,suspend=y,address=5005") .build(); } diff --git a/tests/src/test/java/org/sonarqube/tests/webhook/WebhooksTest.java b/tests/src/test/java/org/sonarqube/tests/webhook/WebhooksTest.java index 92c6bed436b..0bbed529135 100644 --- a/tests/src/test/java/org/sonarqube/tests/webhook/WebhooksTest.java +++ b/tests/src/test/java/org/sonarqube/tests/webhook/WebhooksTest.java @@ -247,8 +247,9 @@ public class WebhooksTest { waitUntilAllWebHooksCalled(1); externalServer.clear(); - // change an issue to blocker bug - Issue firstIssue = tester.wsClient().issues().search(new SearchRequest()).getIssues(0); + // change an issue to blocker bug, QG status goes from OK to ERROR, so webhook is called + List issues = tester.wsClient().issues().search(new SearchRequest()).getIssuesList(); + Issue firstIssue = issues.iterator().next(); tester.wsClient().issues().bulkChange(new BulkChangeRequest().setIssues(singletonList(firstIssue.getKey())) .setSetSeverity(singletonList("BLOCKER")) .setSetType(singletonList("BUG"))); @@ -268,6 +269,22 @@ public class WebhooksTest { assertThat(gate.get("name")).isEqualTo(qGate.getName()); assertThat(gate.get("status")).isEqualTo("ERROR"); assertThat(gate.get("conditions")).isNotNull(); + externalServer.clear(); + + // change severity of issue, won't change the QG status, so no webhook called + tester.wsClient().issues().bulkChange(new BulkChangeRequest().setIssues(singletonList(firstIssue.getKey())) + .setSetSeverity(singletonList("MINOR"))); + waitUntilAllWebHooksCalled(1); + assertThat(externalServer.getPayloadRequests()).isEmpty(); + + // resolve issue as won't fix, QG status goes to OK, so webhook called + tester.wsClient().issues().bulkChange(new BulkChangeRequest().setIssues(singletonList(firstIssue.getKey())) + .setDoTransition("wontfix")); + waitUntilAllWebHooksCalled(1); + request = externalServer.getPayloadRequests().get(0); + payload = jsonToMap(request.getJson()); + gate = (Map) payload.get("qualityGate"); + assertThat(gate.get("status")).isEqualTo("OK"); } private void analyseProject() { -- 2.39.5