From: Julien Lancelot Date: Tue, 10 May 2016 15:49:20 +0000 (+0200) Subject: SONAR-6336 Fail to update or create a condition that already exists X-Git-Tag: 5.6-RC1~158 X-Git-Url: https://source.dussan.org/?a=commitdiff_plain;h=25917fa9613242de25c276889a759760e7cc39bf;p=sonarqube.git SONAR-6336 Fail to update or create a condition that already exists --- diff --git a/server/sonar-server/src/main/java/org/sonar/server/qualitygate/QualityGates.java b/server/sonar-server/src/main/java/org/sonar/server/qualitygate/QualityGates.java index 8f1c65dbc50..6df826fda8f 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/qualitygate/QualityGates.java +++ b/server/sonar-server/src/main/java/org/sonar/server/qualitygate/QualityGates.java @@ -24,8 +24,10 @@ import com.google.common.base.Strings; import com.google.common.collect.Collections2; import java.util.Collection; import javax.annotation.CheckForNull; +import javax.annotation.Nonnull; import javax.annotation.Nullable; import org.apache.commons.lang.BooleanUtils; +import org.apache.commons.lang.ObjectUtils; import org.apache.commons.lang.StringUtils; import org.apache.ibatis.session.SqlSession; import org.sonar.api.config.Settings; @@ -54,6 +56,9 @@ import org.sonar.server.exceptions.ServerException; import org.sonar.server.user.UserSession; import org.sonar.server.util.Validation; +import static com.google.common.collect.FluentIterable.from; +import static java.lang.String.format; + /** * @since 4.3 */ @@ -71,7 +76,7 @@ public class QualityGates { private final Settings settings; public QualityGates(QualityGateDao dao, QualityGateConditionDao conditionDao, MetricFinder metricFinder, PropertiesDao propertiesDao, ComponentDao componentDao, - MyBatis myBatis, UserSession userSession, Settings settings) { + MyBatis myBatis, UserSession userSession, Settings settings) { this.dao = dao; this.conditionDao = conditionDao; this.metricFinder = metricFinder; @@ -117,10 +122,9 @@ public class QualityGates { dao.insert(destinationGate, session); for (QualityGateConditionDto sourceCondition : conditionDao.selectForQualityGate(sourceId, session)) { conditionDao.insert(new QualityGateConditionDto().setQualityGateId(destinationGate.getId()) - .setMetricId(sourceCondition.getMetricId()).setOperator(sourceCondition.getOperator()) - .setWarningThreshold(sourceCondition.getWarningThreshold()).setErrorThreshold(sourceCondition.getErrorThreshold()).setPeriod(sourceCondition.getPeriod()), - session - ); + .setMetricId(sourceCondition.getMetricId()).setOperator(sourceCondition.getOperator()) + .setWarningThreshold(sourceCondition.getWarningThreshold()).setErrorThreshold(sourceCondition.getErrorThreshold()).setPeriod(sourceCondition.getPeriod()), + session); } session.commit(); } finally { @@ -172,11 +176,11 @@ public class QualityGates { } public QualityGateConditionDto createCondition(long qGateId, String metricKey, String operator, - @Nullable String warningThreshold, @Nullable String errorThreshold, @Nullable Integer period) { + @Nullable String warningThreshold, @Nullable String errorThreshold, @Nullable Integer period) { checkPermission(); getNonNullQgate(qGateId); Metric metric = getNonNullMetric(metricKey); - validateCondition(metric, operator, warningThreshold, errorThreshold, period); + validateCondition(qGateId, metric, operator, warningThreshold, errorThreshold, period); QualityGateConditionDto newCondition = new QualityGateConditionDto().setQualityGateId(qGateId) .setMetricId(metric.getId()).setMetricKey(metric.getKey()) .setOperator(operator) @@ -188,11 +192,11 @@ public class QualityGates { } public QualityGateConditionDto updateCondition(long condId, String metricKey, String operator, - @Nullable String warningThreshold, @Nullable String errorThreshold, @Nullable Integer period) { + @Nullable String warningThreshold, @Nullable String errorThreshold, @Nullable Integer period) { checkPermission(); QualityGateConditionDto condition = getNonNullCondition(condId); Metric metric = getNonNullMetric(metricKey); - validateCondition(metric, operator, warningThreshold, errorThreshold, period); + validateCondition(condition.getQualityGateId(), metric, operator, warningThreshold, errorThreshold, period); condition .setMetricId(metric.getId()) .setMetricKey(metric.getKey()) @@ -263,17 +267,31 @@ public class QualityGates { return hasWritePermission; } - private void validateCondition(Metric metric, String operator, @Nullable String warningThreshold, @Nullable String errorThreshold, @Nullable Integer period) { + private void validateCondition(long qGateId, Metric metric, String operator, @Nullable String warningThreshold, @Nullable String errorThreshold, @Nullable Integer period) { Errors errors = new Errors(); validateMetric(metric, errors); checkOperator(metric, operator, errors); checkThresholds(warningThreshold, errorThreshold, errors); checkPeriod(metric, period, errors); + checkConditionDoesNotAlreadyExistOnSameMetricAndPeriod(qGateId, metric, period, errors); if (!errors.isEmpty()) { throw new BadRequestException(errors); } } + private void checkConditionDoesNotAlreadyExistOnSameMetricAndPeriod(long qGateId, Metric metric, @Nullable final Integer period, Errors errors) { + Collection conditions = conditionDao.selectForQualityGate(qGateId); + if (conditions.isEmpty()) { + return; + } + + boolean conditionExists = from(conditions).anyMatch(new MatchMetricAndPeriod(metric.getId(), period)); + String errorMessage = period == null + ? format("Condition on metric '%s' already exists.", metric.getName()) + : format("Condition on metric '%s' over leak period already exists.", metric.getName()); + errors.check(!conditionExists, errorMessage); + } + private void checkPeriod(Metric metric, @Nullable Integer period, Errors errors) { if (period == null) { errors.check(!metric.getKey().startsWith("new_"), "A period must be selected for differential metrics."); @@ -288,11 +306,11 @@ public class QualityGates { private void checkOperator(Metric metric, String operator, Errors errors) { errors - .check(QualityGateConditionDto.isOperatorAllowed(operator, metric.getType()), String.format("Operator %s is not allowed for metric type %s.", operator, metric.getType())); + .check(QualityGateConditionDto.isOperatorAllowed(operator, metric.getType()), format("Operator %s is not allowed for metric type %s.", operator, metric.getType())); } private void validateMetric(Metric metric, Errors errors) { - errors.check(isAlertable(metric), String.format("Metric '%s' cannot be used to define a condition.", metric.getKey())); + errors.check(isAlertable(metric), format("Metric '%s' cannot be used to define a condition.", metric.getKey())); } private boolean isAvailableForInit(Metric metric) { @@ -364,7 +382,6 @@ public class QualityGates { QualityGateDto existingQgate = dao.selectByName(name); boolean isModifyingCurrentQgate = updatingQgateId != null && existingQgate != null && existingQgate.getId().equals(updatingQgateId); errors.check(isModifyingCurrentQgate || existingQgate == null, Validation.IS_ALREADY_USED_MESSAGE, "Name"); - } private void checkPermission() { @@ -377,4 +394,22 @@ public class QualityGates { throw new ForbiddenException("Insufficient privileges"); } } + + private static class MatchMetricAndPeriod implements Predicate { + private final int metricId; + @CheckForNull + private final Integer period; + + public MatchMetricAndPeriod(int metricId, @Nullable Integer period) { + this.metricId = metricId; + this.period = period; + } + + @Override + public boolean apply(@Nonnull QualityGateConditionDto input) { + return input.getMetricId() == metricId && + ObjectUtils.equals(input.getPeriod(), period); + } + } + } diff --git a/server/sonar-server/src/test/java/org/sonar/server/qualitygate/QualityGatesTest.java b/server/sonar-server/src/test/java/org/sonar/server/qualitygate/QualityGatesTest.java index fb610571c13..1d7d0ac1102 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/qualitygate/QualityGatesTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/qualitygate/QualityGatesTest.java @@ -27,6 +27,7 @@ import java.util.List; import org.junit.Before; import org.junit.Rule; import org.junit.Test; +import org.junit.rules.ExpectedException; import org.junit.runner.RunWith; import org.mockito.ArgumentCaptor; import org.mockito.Mock; @@ -59,6 +60,7 @@ import org.sonar.server.tester.UserSessionRule; import org.sonar.server.user.AnonymousUserSession; import org.sonar.server.user.UserSession; +import static java.util.Collections.singletonList; import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.Matchers.any; import static org.mockito.Matchers.anyLong; @@ -72,6 +74,12 @@ import static org.mockito.Mockito.when; @RunWith(MockitoJUnitRunner.class) public class QualityGatesTest { + static final long QUALITY_GATE_ID = 42L; + static final int METRIC_ID = 10; + + @Rule + public ExpectedException expectedException = ExpectedException.none(); + @Rule public UserSessionRule userSessionRule = UserSessionRule.standalone(); @@ -98,7 +106,7 @@ public class QualityGatesTest { Settings settings = new Settings(); - QualityGates qGates; + QualityGates underTest; static final String PROJECT_KEY = "SonarQube"; @@ -114,7 +122,7 @@ public class QualityGatesTest { when(componentDao.selectOrFailById(eq(session), anyLong())).thenReturn(new ComponentDto().setId(1L).setKey(PROJECT_KEY)); when(myBatis.openSession(false)).thenReturn(session); - qGates = new QualityGates(dao, conditionDao, metricFinder, propertiesDao, componentDao, myBatis, userSessionRule, settings); + underTest = new QualityGates(dao, conditionDao, metricFinder, propertiesDao, componentDao, myBatis, userSessionRule, settings); userSessionRule.set(authorizedProfileAdminUserSession); } @@ -122,76 +130,76 @@ public class QualityGatesTest { public void should_list_qgates() { List allQgates = Lists.newArrayList(new QualityGateDto().setName("Gate One"), new QualityGateDto().setName("Gate Two")); when(dao.selectAll()).thenReturn(allQgates); - assertThat(qGates.list()).isEqualTo(allQgates); + assertThat(underTest.list()).isEqualTo(allQgates); } @Test public void should_create_qgate() { String name = "SG-1"; - QualityGateDto sg1 = qGates.create(name); + QualityGateDto sg1 = underTest.create(name); assertThat(sg1.getName()).isEqualTo(name); verify(dao).selectByName(name); verify(dao).insert(sg1); - assertThat(qGates.currentUserHasWritePermission()).isTrue(); + assertThat(underTest.currentUserHasWritePermission()).isTrue(); } @Test(expected = ForbiddenException.class) public void should_fail_create_on_anonymous() { userSessionRule.set(unauthenticatedUserSession); - assertThat(qGates.currentUserHasWritePermission()).isFalse(); - qGates.create("polop"); + assertThat(underTest.currentUserHasWritePermission()).isFalse(); + underTest.create("polop"); } @Test(expected = ForbiddenException.class) public void should_fail_create_on_missing_permission() { userSessionRule.set(unauthorizedUserSession); - qGates.create("polop"); + underTest.create("polop"); } @Test(expected = BadRequestException.class) public void should_fail_create_on_empty_name() { - qGates.create(""); + underTest.create(""); } @Test(expected = BadRequestException.class) public void should_fail_create_on_duplicate_name() { String name = "SG-1"; - when(dao.selectByName(name)).thenReturn(new QualityGateDto().setName(name).setId(42L)); - qGates.create(name); + when(dao.selectByName(name)).thenReturn(new QualityGateDto().setName(name).setId(QUALITY_GATE_ID)); + underTest.create(name); } @Test public void should_get_qgate_by_id() { - long id = 42L; + long id = QUALITY_GATE_ID; final String name = "Golden"; QualityGateDto existing = new QualityGateDto().setId(id).setName(name); when(dao.selectById(id)).thenReturn(existing); - assertThat(qGates.get(id)).isEqualTo(existing); + assertThat(underTest.get(id)).isEqualTo(existing); verify(dao).selectById(id); } @Test public void should_get_qgate_by_name() { - long id = 42L; + long id = QUALITY_GATE_ID; final String name = "Golden"; QualityGateDto existing = new QualityGateDto().setId(id).setName(name); when(dao.selectByName(name)).thenReturn(existing); - assertThat(qGates.get(name)).isEqualTo(existing); + assertThat(underTest.get(name)).isEqualTo(existing); verify(dao).selectByName(name); } @Test(expected = NotFoundException.class) public void should_fail_to_find_qgate_by_name() { - qGates.get("Does not exist"); + underTest.get("Does not exist"); } @Test public void should_rename_qgate() { - long id = 42L; + long id = QUALITY_GATE_ID; String name = "SG-1"; QualityGateDto existing = new QualityGateDto().setId(id).setName("Golden"); when(dao.selectById(id)).thenReturn(existing); - QualityGateDto sg1 = qGates.rename(id, name); + QualityGateDto sg1 = underTest.rename(id, name); assertThat(sg1.getName()).isEqualTo(name); verify(dao).selectById(id); verify(dao).selectByName(name); @@ -200,11 +208,11 @@ public class QualityGatesTest { @Test public void should_allow_rename_with_same_name() { - long id = 42L; + long id = QUALITY_GATE_ID; String name = "SG-1"; QualityGateDto existing = new QualityGateDto().setId(id).setName(name); when(dao.selectById(id)).thenReturn(existing); - QualityGateDto sg1 = qGates.rename(id, name); + QualityGateDto sg1 = underTest.rename(id, name); assertThat(sg1.getName()).isEqualTo(name); verify(dao).selectById(id); verify(dao).selectByName(name); @@ -213,26 +221,26 @@ public class QualityGatesTest { @Test(expected = NotFoundException.class) public void should_fail_rename_on_inexistent_qgate() { - qGates.rename(42L, "Unknown"); + underTest.rename(QUALITY_GATE_ID, "Unknown"); } @Test(expected = BadRequestException.class) public void should_fail_rename_on_duplicate_name() { - long id = 42L; + long id = QUALITY_GATE_ID; String name = "SG-1"; QualityGateDto existing = new QualityGateDto().setId(id).setName("Golden"); when(dao.selectById(id)).thenReturn(existing); when(dao.selectByName(name)).thenReturn(new QualityGateDto().setId(666L).setName(name)); - qGates.rename(id, name); + underTest.rename(id, name); } @Test public void should_select_default_qgate_and_update_settings() { - long defaultId = 42L; + long defaultId = QUALITY_GATE_ID; String defaultName = "Default Name"; when(dao.selectById(defaultId)).thenReturn(new QualityGateDto().setId(defaultId).setName(defaultName)); - qGates.setDefault(defaultId); + underTest.setDefault(defaultId); verify(dao).selectById(defaultId); ArgumentCaptor propertyCaptor = ArgumentCaptor.forClass(PropertyDto.class); @@ -247,7 +255,7 @@ public class QualityGatesTest { public void should_unset_default_qgate_and_unset_in_settings() { settings.setProperty("sonar.qualitygate", 42l); - qGates.setDefault(null); + underTest.setDefault(null); verify(propertiesDao).deleteGlobalProperty("sonar.qualitygate"); assertThat(settings.hasKey("sonar.qualitygate")).isFalse(); @@ -255,13 +263,13 @@ public class QualityGatesTest { @Test public void should_delete_qgate() { - long idToDelete = 42L; + long idToDelete = QUALITY_GATE_ID; String name = "To Delete"; QualityGateDto toDelete = new QualityGateDto().setId(idToDelete).setName(name); when(dao.selectById(idToDelete)).thenReturn(toDelete); DbSession session = mock(DbSession.class); when(myBatis.openSession(false)).thenReturn(session); - qGates.delete(idToDelete); + underTest.delete(idToDelete); verify(dao).selectById(idToDelete); verify(propertiesDao).deleteProjectProperties("sonar.qualitygate", "42", session); verify(dao).delete(toDelete, session); @@ -269,14 +277,14 @@ public class QualityGatesTest { @Test public void should_delete_qgate_if_non_default() { - long idToDelete = 42L; + long idToDelete = QUALITY_GATE_ID; String name = "To Delete"; QualityGateDto toDelete = new QualityGateDto().setId(idToDelete).setName(name); when(dao.selectById(idToDelete)).thenReturn(toDelete); when(propertiesDao.selectGlobalProperty("sonar.qualitygate")).thenReturn(new PropertyDto().setValue("666")); DbSession session = mock(DbSession.class); when(myBatis.openSession(false)).thenReturn(session); - qGates.delete(idToDelete); + underTest.delete(idToDelete); verify(dao).selectById(idToDelete); verify(propertiesDao).deleteProjectProperties("sonar.qualitygate", "42", session); verify(dao).delete(toDelete, session); @@ -284,14 +292,14 @@ public class QualityGatesTest { @Test public void should_delete_qgate_even_if_default() { - long idToDelete = 42L; + long idToDelete = QUALITY_GATE_ID; String name = "To Delete"; QualityGateDto toDelete = new QualityGateDto().setId(idToDelete).setName(name); when(dao.selectById(idToDelete)).thenReturn(toDelete); when(propertiesDao.selectGlobalProperty("sonar.qualitygate")).thenReturn(new PropertyDto().setValue("42")); DbSession session = mock(DbSession.class); when(myBatis.openSession(false)).thenReturn(session); - qGates.delete(idToDelete); + underTest.delete(idToDelete); verify(dao).selectById(idToDelete); verify(propertiesDao).deleteGlobalProperty("sonar.qualitygate", session); verify(propertiesDao).deleteProjectProperties("sonar.qualitygate", "42", session); @@ -301,22 +309,22 @@ public class QualityGatesTest { @Test public void should_return_default_qgate_if_set() { String defaultName = "Sonar way"; - long defaultId = 42L; + long defaultId = QUALITY_GATE_ID; when(propertiesDao.selectGlobalProperty("sonar.qualitygate")).thenReturn(new PropertyDto().setValue(Long.toString(defaultId))); QualityGateDto defaultQgate = new QualityGateDto().setId(defaultId).setName(defaultName); when(dao.selectById(defaultId)).thenReturn(defaultQgate); - assertThat(qGates.getDefault()).isEqualTo(defaultQgate); + assertThat(underTest.getDefault()).isEqualTo(defaultQgate); } @Test public void should_return_null_default_qgate_if_unset() { when(propertiesDao.selectGlobalProperty("sonar.qualitygate")).thenReturn(new PropertyDto().setValue("")); - assertThat(qGates.getDefault()).isNull(); + assertThat(underTest.getDefault()).isNull(); } @Test public void should_create_warning_condition_without_period() { - long qGateId = 42L; + long qGateId = QUALITY_GATE_ID; String metricKey = "coverage"; String operator = "LT"; String warningThreshold = "90"; @@ -326,7 +334,7 @@ public class QualityGatesTest { when(coverage.getId()).thenReturn(metricId); when(metricFinder.findByKey(metricKey)).thenReturn(coverage); - QualityGateConditionDto newCondition = qGates.createCondition(qGateId, metricKey, operator, warningThreshold, null, null); + QualityGateConditionDto newCondition = underTest.createCondition(qGateId, metricKey, operator, warningThreshold, null, null); assertThat(newCondition.getQualityGateId()).isEqualTo(qGateId); assertThat(newCondition.getMetricId()).isEqualTo((long) metricId); assertThat(newCondition.getOperator()).isEqualTo(operator); @@ -338,7 +346,7 @@ public class QualityGatesTest { @Test public void should_create_error_condition_with_period() { - long qGateId = 42L; + long qGateId = QUALITY_GATE_ID; String metricKey = "new_coverage"; String operator = "LT"; String errorThreshold = "80"; @@ -349,7 +357,7 @@ public class QualityGatesTest { when(metricFinder.findByKey(metricKey)).thenReturn(newCoverage); int period = 1; - QualityGateConditionDto newCondition = qGates.createCondition(qGateId, metricKey, operator, null, errorThreshold, period); + QualityGateConditionDto newCondition = underTest.createCondition(qGateId, metricKey, operator, null, errorThreshold, period); assertThat(newCondition.getQualityGateId()).isEqualTo(qGateId); assertThat(newCondition.getMetricId()).isEqualTo((long) metricId); assertThat(newCondition.getMetricKey()).isEqualTo(metricKey); @@ -362,95 +370,122 @@ public class QualityGatesTest { @Test(expected = NotFoundException.class) public void should_fail_create_condition_on_missing_metric() { - long qGateId = 42L; + long qGateId = QUALITY_GATE_ID; when(dao.selectById(qGateId)).thenReturn(new QualityGateDto().setId(qGateId)); - qGates.createCondition(qGateId, "new_coverage", "LT", null, "80", 2); + underTest.createCondition(qGateId, "new_coverage", "LT", null, "80", 2); } @Test(expected = BadRequestException.class) public void should_fail_create_condition_on_alert_metric() { - long qGateId = 42L; + long qGateId = QUALITY_GATE_ID; when(metricFinder.findByKey(anyString())).thenReturn(CoreMetrics.ALERT_STATUS); when(dao.selectById(qGateId)).thenReturn(new QualityGateDto().setId(qGateId)); - qGates.createCondition(qGateId, "alert_status", "EQ", null, "80", 2); + underTest.createCondition(qGateId, "alert_status", "EQ", null, "80", 2); } @Test(expected = BadRequestException.class) public void should_fail_create_condition_on_non_data_metric() { - long qGateId = 42L; + long qGateId = QUALITY_GATE_ID; final Metric metric = mock(Metric.class); when(metric.getType()).thenReturn(ValueType.DATA); when(metricFinder.findByKey(anyString())).thenReturn(metric); when(dao.selectById(qGateId)).thenReturn(new QualityGateDto().setId(qGateId)); - qGates.createCondition(qGateId, "alert_status", "LT", null, "80", 2); + underTest.createCondition(qGateId, "alert_status", "LT", null, "80", 2); } @Test(expected = BadRequestException.class) public void should_fail_create_condition_on_hidden_metric() { - long qGateId = 42L; + long qGateId = QUALITY_GATE_ID; final Metric metric = mock(Metric.class); when(metric.isHidden()).thenReturn(true); when(metric.getType()).thenReturn(ValueType.INT); when(metricFinder.findByKey(anyString())).thenReturn(metric); when(dao.selectById(qGateId)).thenReturn(new QualityGateDto().setId(qGateId)); - qGates.createCondition(qGateId, "alert_status", "LT", null, "80", 2); + underTest.createCondition(qGateId, "alert_status", "LT", null, "80", 2); } @Test(expected = BadRequestException.class) public void should_fail_create_condition_on_rating_metric() { - long qGateId = 42L; + long qGateId = QUALITY_GATE_ID; final Metric metric = mock(Metric.class); when(metric.getType()).thenReturn(ValueType.RATING); when(metricFinder.findByKey(anyString())).thenReturn(metric); when(dao.selectById(qGateId)).thenReturn(new QualityGateDto().setId(qGateId)); - qGates.createCondition(qGateId, "alert_status", "LT", null, "80", 2); + underTest.createCondition(qGateId, "alert_status", "LT", null, "80", 2); } @Test(expected = BadRequestException.class) public void should_fail_create_condition_on_unallowed_operator() { - long qGateId = 42L; + long qGateId = QUALITY_GATE_ID; final Metric metric = mock(Metric.class); when(metric.getType()).thenReturn(ValueType.BOOL); when(metricFinder.findByKey(anyString())).thenReturn(metric); when(dao.selectById(qGateId)).thenReturn(new QualityGateDto().setId(qGateId)); - qGates.createCondition(qGateId, "alert_status", "LT", null, "80", 2); + underTest.createCondition(qGateId, "alert_status", "LT", null, "80", 2); } @Test(expected = BadRequestException.class) public void should_fail_create_condition_on_missing_thresholds() { - long qGateId = 42L; + long qGateId = QUALITY_GATE_ID; final Metric metric = mock(Metric.class); when(metric.getType()).thenReturn(ValueType.BOOL); when(metricFinder.findByKey(anyString())).thenReturn(metric); when(dao.selectById(qGateId)).thenReturn(new QualityGateDto().setId(qGateId)); - qGates.createCondition(qGateId, "alert_status", "EQ", null, null, 2); + underTest.createCondition(qGateId, "alert_status", "EQ", null, null, 2); } @Test(expected = BadRequestException.class) public void should_fail_create_condition_on_missing_period() { - long qGateId = 42L; + long qGateId = QUALITY_GATE_ID; final Metric metric = mock(Metric.class); when(metric.getKey()).thenReturn("new_coverage"); when(metric.getType()).thenReturn(ValueType.BOOL); when(metricFinder.findByKey(anyString())).thenReturn(metric); when(dao.selectById(qGateId)).thenReturn(new QualityGateDto().setId(qGateId)); - qGates.createCondition(qGateId, "alert_status", "EQ", null, "90", null); + underTest.createCondition(qGateId, "alert_status", "EQ", null, "90", null); } @Test(expected = BadRequestException.class) public void should_fail_create_condition_on_invalid_period() { - long qGateId = 42L; + long qGateId = QUALITY_GATE_ID; final Metric metric = mock(Metric.class); when(metric.getKey()).thenReturn("new_coverage"); when(metric.getType()).thenReturn(ValueType.BOOL); when(metricFinder.findByKey(anyString())).thenReturn(metric); when(dao.selectById(qGateId)).thenReturn(new QualityGateDto().setId(qGateId)); - qGates.createCondition(qGateId, "alert_status", "EQ", null, "90", 6); + underTest.createCondition(qGateId, "alert_status", "EQ", null, "90", 6); + } + + @Test + public void fail_to_create_condition_when_condition_on_same_metric_already_exist() throws Exception { + String metricKey = "coverage"; + addMetric(metricKey, "Coverage"); + when(dao.selectById(QUALITY_GATE_ID)).thenReturn(new QualityGateDto().setId(QUALITY_GATE_ID)); + when(conditionDao.selectForQualityGate(QUALITY_GATE_ID)).thenReturn( + singletonList(new QualityGateConditionDto().setMetricKey(metricKey).setMetricId(METRIC_ID))); + + expectedException.expect(BadRequestException.class); + expectedException.expectMessage("Condition on metric 'Coverage' already exists."); + underTest.createCondition(QUALITY_GATE_ID, metricKey, "LT", "90", null, null); + } + + @Test + public void fail_to_create_condition_when_condition_on_same_metric_and_period_already_exist() throws Exception { + String metricKey = "new_coverage"; + addMetric(metricKey, "New Coverage"); + when(dao.selectById(QUALITY_GATE_ID)).thenReturn(new QualityGateDto().setId(QUALITY_GATE_ID)); + + when(conditionDao.selectForQualityGate(QUALITY_GATE_ID)).thenReturn( + singletonList(new QualityGateConditionDto().setMetricKey(metricKey).setMetricId(METRIC_ID).setPeriod(1))); + + expectedException.expect(BadRequestException.class); + expectedException.expectMessage("Condition on metric 'New Coverage' over leak period already exists."); + underTest.createCondition(QUALITY_GATE_ID, metricKey, "LT", "90", null, 1); } @Test public void should_update_condition() { - long condId = 42L; + long condId = QUALITY_GATE_ID; String metricKey = "new_coverage"; String operator = "LT"; String errorThreshold = "80"; @@ -463,7 +498,7 @@ public class QualityGatesTest { when(metricFinder.findByKey(metricKey)).thenReturn(newCoverage); int period = 1; - assertThat(qGates.updateCondition(condId, metricKey, operator, null, errorThreshold, period)).isEqualTo(condition); + assertThat(underTest.updateCondition(condId, metricKey, operator, null, errorThreshold, period)).isEqualTo(condition); assertThat(condition.getId()).isEqualTo(condId); assertThat(condition.getMetricId()).isEqualTo((long) metricId); assertThat(condition.getMetricKey()).isEqualTo(metricKey); @@ -476,7 +511,7 @@ public class QualityGatesTest { @Test public void should_list_conditions() { - long qGateId = 42L; + long qGateId = QUALITY_GATE_ID; long metric1Id = 1L; String metric1Key = "polop"; long metric2Id = 2L; @@ -491,7 +526,7 @@ public class QualityGatesTest { Metric metric2 = mock(Metric.class); when(metric2.getKey()).thenReturn(metric2Key); when(metricFinder.findById((int) metric2Id)).thenReturn(metric2); - assertThat(qGates.listConditions(qGateId)).isEqualTo(conditions); + assertThat(underTest.listConditions(qGateId)).isEqualTo(conditions); Iterator iterator = conditions.iterator(); assertThat(iterator.next().getMetricKey()).isEqualTo(metric1Key); assertThat(iterator.next().getMetricKey()).isEqualTo(metric2Key); @@ -499,7 +534,7 @@ public class QualityGatesTest { @Test(expected = IllegalStateException.class) public void should_do_a_sanity_check_when_listing_conditions() { - long qGateId = 42L; + long qGateId = QUALITY_GATE_ID; long metric1Id = 1L; String metric1Key = "polop"; long metric2Id = 2L; @@ -510,30 +545,30 @@ public class QualityGatesTest { Metric metric1 = mock(Metric.class); when(metric1.getKey()).thenReturn(metric1Key); when(metricFinder.findById((int) metric1Id)).thenReturn(metric1); - qGates.listConditions(qGateId); + underTest.listConditions(qGateId); } @Test public void should_delete_condition() { - long idToDelete = 42L; + long idToDelete = QUALITY_GATE_ID; QualityGateConditionDto toDelete = new QualityGateConditionDto().setId(idToDelete); when(conditionDao.selectById(idToDelete)).thenReturn(toDelete); - qGates.deleteCondition(idToDelete); + underTest.deleteCondition(idToDelete); verify(conditionDao).selectById(idToDelete); verify(conditionDao).delete(toDelete); } @Test(expected = NotFoundException.class) public void should_fail_delete_condition() { - qGates.deleteCondition(42L); + underTest.deleteCondition(QUALITY_GATE_ID); } @Test public void should_associate_project() { - Long qGateId = 42L; + Long qGateId = QUALITY_GATE_ID; Long projectId = 24L; when(dao.selectById(qGateId)).thenReturn(new QualityGateDto().setId(qGateId)); - qGates.associateProject(qGateId, projectId); + underTest.associateProject(qGateId, projectId); verify(dao).selectById(qGateId); ArgumentCaptor propertyCaptor = ArgumentCaptor.forClass(PropertyDto.class); verify(propertiesDao).insertProperty(propertyCaptor.capture()); @@ -547,10 +582,10 @@ public class QualityGatesTest { public void associate_project_with_project_admin_permission() { userSessionRule.set(authorizedProjectAdminUserSession); - Long qGateId = 42L; + Long qGateId = QUALITY_GATE_ID; Long projectId = 24L; when(dao.selectById(qGateId)).thenReturn(new QualityGateDto().setId(qGateId)); - qGates.associateProject(qGateId, projectId); + underTest.associateProject(qGateId, projectId); verify(dao).selectById(qGateId); ArgumentCaptor propertyCaptor = ArgumentCaptor.forClass(PropertyDto.class); verify(propertiesDao).insertProperty(propertyCaptor.capture()); @@ -562,10 +597,10 @@ public class QualityGatesTest { @Test public void should_dissociate_project() { - Long qGateId = 42L; + Long qGateId = QUALITY_GATE_ID; Long projectId = 24L; when(dao.selectById(qGateId)).thenReturn(new QualityGateDto().setId(qGateId)); - qGates.dissociateProject(qGateId, projectId); + underTest.dissociateProject(qGateId, projectId); verify(dao).selectById(qGateId); verify(propertiesDao).deleteProjectProperty("sonar.qualitygate", projectId); } @@ -574,10 +609,10 @@ public class QualityGatesTest { public void dissociate_project_with_project_admin_permission() { userSessionRule.set(authorizedProjectAdminUserSession); - Long qGateId = 42L; + Long qGateId = QUALITY_GATE_ID; Long projectId = 24L; when(dao.selectById(qGateId)).thenReturn(new QualityGateDto().setId(qGateId)); - qGates.dissociateProject(qGateId, projectId); + underTest.dissociateProject(qGateId, projectId); verify(dao).selectById(qGateId); verify(propertiesDao).deleteProjectProperty("sonar.qualitygate", projectId); } @@ -585,7 +620,7 @@ public class QualityGatesTest { @Test public void should_copy_qgate() { String name = "Atlantis"; - long sourceId = 42L; + long sourceId = QUALITY_GATE_ID; final long destId = 43L; long metric1Id = 1L; long metric2Id = 2L; @@ -604,7 +639,7 @@ public class QualityGatesTest { } }).when(dao).insert(any(QualityGateDto.class), eq(session)); when(conditionDao.selectForQualityGate(anyLong(), eq(session))).thenReturn(conditions); - QualityGateDto atlantis = qGates.copy(sourceId, name); + QualityGateDto atlantis = underTest.copy(sourceId, name); assertThat(atlantis.getName()).isEqualTo(name); verify(dao).selectByName(name); verify(dao).insert(atlantis, session); @@ -627,6 +662,14 @@ public class QualityGatesTest { when(classicMetric.getType()).thenReturn(ValueType.BOOL); when(metricFinder.findAll()).thenReturn(ImmutableList.of( dataMetric, hiddenMetric, nullHiddenMetric, alertMetric, ratingMetric, classicMetric)); - assertThat(qGates.gateMetrics()).hasSize(3).containsOnly(classicMetric, hiddenMetric, nullHiddenMetric); + assertThat(underTest.gateMetrics()).hasSize(3).containsOnly(classicMetric, hiddenMetric, nullHiddenMetric); + } + + private Metric addMetric(String metricKey, String metricName) { + Metric metric = Mockito.spy(CoreMetrics.COVERAGE); + when(metric.getId()).thenReturn(METRIC_ID); + when(metric.getName()).thenReturn(metricName); + when(metricFinder.findByKey(metricKey)).thenReturn(metric); + return metric; } }