SONAR-6336 Add check when updating condition

When updating a condition, if new new condition value match existing condition, it should fail

2 files changed, 64 insertions, 13 deletions

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 1bd05457571..e042e83cb75 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
@@ -181,7 +181,7 @@ public class QualityGates {
     getNonNullQgate(qGateId);
     Metric metric = getNonNullMetric(metricKey);
     validateCondition(metric, operator, warningThreshold, errorThreshold, period);
-    checkConditionDoesNotAlreadyExistOnSameMetricAndPeriod(qGateId, metric, period);
+    checkConditionDoesNotAlreadyExistOnSameMetricAndPeriod(getConditions(qGateId, null), metric, period);
     QualityGateConditionDto newCondition = new QualityGateConditionDto().setQualityGateId(qGateId)
       .setMetricId(metric.getId()).setMetricKey(metric.getKey())
       .setOperator(operator)
@@ -198,6 +198,7 @@ public class QualityGates {
     QualityGateConditionDto condition = getNonNullCondition(condId);
     Metric metric = getNonNullMetric(metricKey);
     validateCondition(metric, operator, warningThreshold, errorThreshold, period);
+    checkConditionDoesNotAlreadyExistOnSameMetricAndPeriod(getConditions(condition.getQualityGateId(), condition.getId()), metric, period);
     condition
       .setMetricId(metric.getId())
       .setMetricKey(metric.getKey())
@@ -268,6 +269,14 @@ public class QualityGates {
     return hasWritePermission;
   }
 
+  private Collection<QualityGateConditionDto> getConditions(long qGateId, @Nullable Long conditionId) {
+    Collection<QualityGateConditionDto> conditions = conditionDao.selectForQualityGate(qGateId);
+    if (conditionId == null) {
+      return conditions;
+    }
+    return from(conditionDao.selectForQualityGate(qGateId)).filter(new MatchConditionId(conditionId)).toList();
+  }
+
   private void validateCondition(Metric metric, String operator, @Nullable String warningThreshold, @Nullable String errorThreshold, @Nullable Integer period) {
     Errors errors = new Errors();
     validateMetric(metric, errors);
@@ -279,8 +288,7 @@ public class QualityGates {
     }
   }
 
-  private void checkConditionDoesNotAlreadyExistOnSameMetricAndPeriod(long qGateId, Metric metric, @Nullable final Integer period) {
-    Collection<QualityGateConditionDto> conditions = conditionDao.selectForQualityGate(qGateId);
+  private void checkConditionDoesNotAlreadyExistOnSameMetricAndPeriod(Collection<QualityGateConditionDto> conditions, Metric metric, @Nullable final Integer period) {
     if (conditions.isEmpty()) {
       return;
     }
@@ -414,4 +422,17 @@ public class QualityGates {
     }
   }
 
+  private static class MatchConditionId implements Predicate<QualityGateConditionDto> {
+    private final long conditionId;
+
+    private MatchConditionId(long conditionId) {
+      this.conditionId = conditionId;
+    }
+
+    @Override
+    public boolean apply(@Nonnull QualityGateConditionDto input) {
+      return input.getId() != conditionId;
+    }
+  }
+
 }
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 e034eabe131..6e32b15d4b3 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
@@ -24,6 +24,8 @@ import com.google.common.collect.Lists;
 import java.util.Collection;
 import java.util.Iterator;
 import java.util.List;
+import org.apache.commons.lang.RandomStringUtils;
+import org.apache.commons.lang.math.RandomUtils;
 import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
@@ -60,6 +62,7 @@ import org.sonar.server.tester.UserSessionRule;
 import org.sonar.server.user.AnonymousUserSession;
 import org.sonar.server.user.UserSession;
 
+import static java.util.Arrays.asList;
 import static java.util.Collections.singletonList;
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.mockito.Matchers.any;
@@ -485,27 +488,36 @@ public class QualityGatesTest {
 
   @Test
   public void should_update_condition() {
-    long condId = 43;
     String metricKey = "new_coverage";
     String operator = "LT";
     String errorThreshold = "80";
     addMetric(metricKey, "New Coverage");
 
-    QualityGateConditionDto condition = new QualityGateConditionDto().setId(condId)
-      .setQualityGateId(QUALITY_GATE_ID)
-      .setMetricId(METRIC_ID)
-      .setMetricKey(metricKey)
-      .setOperator("GT")
-      .setWarningThreshold("123")
-      .setPeriod(1);
-    when(conditionDao.selectById(condId)).thenReturn(condition);
+    QualityGateConditionDto condition = insertQualityGateConditionDto(newCondition(metricKey, METRIC_ID));
     when(conditionDao.selectForQualityGate(QUALITY_GATE_ID)).thenReturn(singletonList(condition));
 
-    assertThat(underTest.updateCondition(condId, metricKey, operator, null, errorThreshold, 1)).isEqualTo(condition);
+    assertThat(underTest.updateCondition(condition.getId(), metricKey, operator, null, errorThreshold, 1)).isEqualTo(condition);
     verify(conditionDao).update(condition);
   }
 
   @Test
+  public void fail_to_update_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));
+
+    QualityGateConditionDto conditionNotOnLeakPeriod = insertQualityGateConditionDto(newCondition(metricKey, METRIC_ID)).setPeriod(0);
+    QualityGateConditionDto conditionOnLeakPeriod = insertQualityGateConditionDto(newCondition(metricKey, METRIC_ID)).setPeriod(1);
+    when(conditionDao.selectForQualityGate(QUALITY_GATE_ID)).thenReturn(asList(conditionNotOnLeakPeriod, conditionOnLeakPeriod));
+
+    expectedException.expect(BadRequestException.class);
+    expectedException.expectMessage("Condition on metric 'Coverage' over leak period already exists.");
+
+    // Update condition not on leak period to be on leak period => will fail as this condition already exist
+    underTest.updateCondition(conditionNotOnLeakPeriod.getId(), metricKey, "LT", null, "60", 1);
+  }
+
+  @Test
   public void should_list_conditions() {
     long qGateId = QUALITY_GATE_ID;
     long metric1Id = 1L;
@@ -668,4 +680,22 @@ public class QualityGatesTest {
     when(metricFinder.findByKey(metricKey)).thenReturn(metric);
     return metric;
   }
+
+  private QualityGateConditionDto newCondition(String metricKey, int metricId) {
+    return new QualityGateConditionDto()
+      .setId(RandomUtils.nextLong())
+      .setMetricKey(metricKey)
+      .setMetricId(metricId)
+      .setQualityGateId(QUALITY_GATE_ID)
+      .setOperator("GT")
+      .setWarningThreshold(RandomStringUtils.randomAlphanumeric(15))
+      .setErrorThreshold(RandomStringUtils.randomAlphanumeric(15))
+      .setPeriod(RandomUtils.nextBoolean() ? 1 : null);
+  }
+
+  private QualityGateConditionDto insertQualityGateConditionDto(QualityGateConditionDto conditionDto) {
+    when(conditionDao.selectById(conditionDto.getId())).thenReturn(conditionDto);
+    return conditionDto;
+  }
+
 }