SONAR-10663 Prevent duplicate rule parameters in the database (#341)

11 files changed, 323 insertions, 9 deletions

diff --git a/server/sonar-db-core/src/main/resources/org/sonar/db/version/schema-h2.ddl b/server/sonar-db-core/src/main/resources/org/sonar/db/version/schema-h2.ddl
index e7c04a5723d..c9ae525b88d 100644
--- a/server/sonar-db-core/src/main/resources/org/sonar/db/version/schema-h2.ddl
+++ b/server/sonar-db-core/src/main/resources/org/sonar/db/version/schema-h2.ddl
@@ -43,6 +43,7 @@ CREATE TABLE "RULES_PARAMETERS" (
   "DESCRIPTION" VARCHAR(4000)
 );
 CREATE INDEX "RULES_PARAMETERS_RULE_ID" ON "RULES_PARAMETERS" ("RULE_ID");
+CREATE UNIQUE INDEX "RULES_PARAMETERS_UNIQUE" ON "RULES_PARAMETERS" ("RULE_ID", "NAME");
 
 
 CREATE TABLE "RULES_PROFILES" (
diff --git a/server/sonar-db-dao/src/test/java/org/sonar/db/rule/RuleDaoTest.java b/server/sonar-db-dao/src/test/java/org/sonar/db/rule/RuleDaoTest.java
index a4f3034566c..a0ae3743e20 100644
--- a/server/sonar-db-dao/src/test/java/org/sonar/db/rule/RuleDaoTest.java
+++ b/server/sonar-db-dao/src/test/java/org/sonar/db/rule/RuleDaoTest.java
@@ -745,19 +745,41 @@ public class RuleDaoTest {
 
   @Test
   public void insert_parameter() {
-    db.prepareDbUnit(getClass(), "insert_parameter.xml");
-    RuleDefinitionDto rule1 = underTest.selectOrFailDefinitionByKey(db.getSession(), RuleKey.of("plugin", "NewRuleKey"));
+    RuleDefinitionDto ruleDefinitionDto = db.rules().insert();
 
-    RuleParamDto param = RuleParamDto.createFor(rule1)
+    RuleParamDto orig = RuleParamDto.createFor(ruleDefinitionDto)
       .setName("max")
       .setType("INTEGER")
       .setDefaultValue("30")
       .setDescription("My Parameter");
 
-    underTest.insertRuleParam(db.getSession(), rule1, param);
-    db.getSession().commit();
+    underTest.insertRuleParam(db.getSession(), ruleDefinitionDto, orig);
+
+    List<RuleParamDto> ruleParamDtos = underTest.selectRuleParamsByRuleKey(db.getSession(), ruleDefinitionDto.getKey());
+    assertThat(ruleParamDtos).hasSize(1);
+
+    RuleParamDto loaded = ruleParamDtos.get(0);
+    assertThat(loaded.getRuleId()).isEqualTo(orig.getRuleId());
+    assertThat(loaded.getName()).isEqualTo(orig.getName());
+    assertThat(loaded.getType()).isEqualTo(orig.getType());
+    assertThat(loaded.getDefaultValue()).isEqualTo(orig.getDefaultValue());
+    assertThat(loaded.getDescription()).isEqualTo(orig.getDescription());
+  }
 
-    db.assertDbUnit(getClass(), "insert_parameter-result.xml", "rules_parameters");
+  @Test
+  public void should_fail_to_insert_duplicate_parameter() {
+    RuleDefinitionDto ruleDefinitionDto = db.rules().insert();
+
+    RuleParamDto param = RuleParamDto.createFor(ruleDefinitionDto)
+      .setName("max")
+      .setType("INTEGER")
+      .setDefaultValue("30")
+      .setDescription("My Parameter");
+
+    underTest.insertRuleParam(db.getSession(), ruleDefinitionDto, param);
+
+    thrown.expect(PersistenceException.class);
+    underTest.insertRuleParam(db.getSession(), ruleDefinitionDto, param);
   }
 
   @Test
diff --git a/server/sonar-db-migration/src/main/java/org/sonar/server/platform/db/migration/version/v72/AddUniqueIndexOnRulesParameters.java b/server/sonar-db-migration/src/main/java/org/sonar/server/platform/db/migration/version/v72/AddUniqueIndexOnRulesParameters.java
new file mode 100644
index 00000000000..cfb48acf321
--- /dev/null
+++ b/server/sonar-db-migration/src/main/java/org/sonar/server/platform/db/migration/version/v72/AddUniqueIndexOnRulesParameters.java
@@ -0,0 +1,52 @@
+/*
+ * SonarQube
+ * Copyright (C) 2009-2018 SonarSource SA
+ * mailto:info AT sonarsource DOT com
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 3 of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ */
+package org.sonar.server.platform.db.migration.version.v72;
+
+import java.sql.SQLException;
+import org.sonar.db.Database;
+import org.sonar.server.platform.db.migration.def.IntegerColumnDef;
+import org.sonar.server.platform.db.migration.def.VarcharColumnDef;
+import org.sonar.server.platform.db.migration.sql.CreateIndexBuilder;
+import org.sonar.server.platform.db.migration.step.DdlChange;
+
+public class AddUniqueIndexOnRulesParameters extends DdlChange {
+
+  public AddUniqueIndexOnRulesParameters(Database db) {
+    super(db);
+  }
+
+  @Override
+  public void execute(Context context) throws SQLException {
+    context.execute(new CreateIndexBuilder(getDialect())
+      .setTable("rules_parameters")
+      .setName("rules_parameters_unique")
+      .setUnique(true)
+      .addColumn(IntegerColumnDef.newIntegerColumnDefBuilder()
+        .setColumnName("rule_id")
+        .setIsNullable(false)
+        .build())
+      .addColumn(VarcharColumnDef.newVarcharColumnDefBuilder()
+        .setColumnName("name")
+        .setLimit(128)
+        .setIsNullable(false)
+        .build())
+      .build());
+  }
+}
diff --git a/server/sonar-db-migration/src/main/java/org/sonar/server/platform/db/migration/version/v72/DbVersion72.java b/server/sonar-db-migration/src/main/java/org/sonar/server/platform/db/migration/version/v72/DbVersion72.java
index 8a8e0aec026..ec0eb63fdd1 100644
--- a/server/sonar-db-migration/src/main/java/org/sonar/server/platform/db/migration/version/v72/DbVersion72.java
+++ b/server/sonar-db-migration/src/main/java/org/sonar/server/platform/db/migration/version/v72/DbVersion72.java
@@ -55,6 +55,8 @@ public class DbVersion72 implements DbVersion {
       .add(2125, "Populate FILE_SOURCE.LINE_COUNT", PopulateFileSourceLineCount.class)
       .add(2126, "Make FILE_SOURCE.LINE_COUNT not nullable", MakeFileSourceLineCountNotNullable.class)
       .add(2127, "Purge orphans for Compute Engine", PurgeOrphansForCE.class)
+      .add(2128, "Purge duplicate rules_parameters and their orphans", PurgeDuplicateRulesParameters.class)
+      .add(2129, "Add unique index on rule_id + name on rules_parameters", AddUniqueIndexOnRulesParameters.class)
     ;
   }
 }
diff --git a/server/sonar-db-migration/src/main/java/org/sonar/server/platform/db/migration/version/v72/PurgeDuplicateRulesParameters.java b/server/sonar-db-migration/src/main/java/org/sonar/server/platform/db/migration/version/v72/PurgeDuplicateRulesParameters.java
new file mode 100644
index 00000000000..12f4867df13
--- /dev/null
+++ b/server/sonar-db-migration/src/main/java/org/sonar/server/platform/db/migration/version/v72/PurgeDuplicateRulesParameters.java
@@ -0,0 +1,67 @@
+/*
+ * SonarQube
+ * Copyright (C) 2009-2018 SonarSource SA
+ * mailto:info AT sonarsource DOT com
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 3 of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ */
+package org.sonar.server.platform.db.migration.version.v72;
+
+import java.sql.SQLException;
+import org.sonar.db.Database;
+import org.sonar.server.platform.db.migration.step.DataChange;
+
+public class PurgeDuplicateRulesParameters extends DataChange {
+
+  private static final String REMOVE_DUPLICATE_RULES_PARAMS_SQL_FOR_GENERIC =
+    "DELETE FROM rules_parameters p1 WHERE id NOT IN (SELECT * FROM (SELECT MIN(id) FROM rules_parameters GROUP BY rule_id, name) temp)";
+
+  private static final String REMOVE_DUPLICATE_ACTIVE_RULE_PARAMS_SQL_FOR_GENERIC =
+    "DELETE FROM active_rule_parameters arp WHERE arp.rules_parameter_id NOT IN (SELECT * FROM (SELECT MIN(id) FROM rules_parameters GROUP BY rule_id, name) temp)";
+
+  private static final String REMOVE_DUPLICATE_RULES_PARAMS_SQL_FOR_MYSQL_MSSQL =
+    "DELETE p1 FROM rules_parameters as p1 WHERE id NOT IN (SELECT id FROM (SELECT MIN(id) as id FROM rules_parameters GROUP BY rule_id, name) temp)";
+
+  private static final String REMOVE_DUPLICATE_ACTIVE_RULE_PARAMS_SQL_FOR_MYSQL_MSSQL =
+    "DELETE arp FROM active_rule_parameters as arp WHERE arp.rules_parameter_id NOT IN (SELECT id FROM (SELECT MIN(id) as id FROM rules_parameters GROUP BY rule_id, name) temp)";
+
+  public PurgeDuplicateRulesParameters(Database db) {
+    super(db);
+  }
+
+  @Override
+  public void execute(Context context) throws SQLException {
+    String removeDuplicateRulesParamsSql;
+    String removeDuplicateActiveRuleParamsSql;
+    switch (getDialect().getId()) {
+      case "mssql":
+      case "mysql":
+        removeDuplicateRulesParamsSql = REMOVE_DUPLICATE_RULES_PARAMS_SQL_FOR_MYSQL_MSSQL;
+        removeDuplicateActiveRuleParamsSql = REMOVE_DUPLICATE_ACTIVE_RULE_PARAMS_SQL_FOR_MYSQL_MSSQL;
+        break;
+      default:
+        removeDuplicateRulesParamsSql = REMOVE_DUPLICATE_RULES_PARAMS_SQL_FOR_GENERIC;
+        removeDuplicateActiveRuleParamsSql = REMOVE_DUPLICATE_ACTIVE_RULE_PARAMS_SQL_FOR_GENERIC;
+        break;
+    }
+
+    context.prepareUpsert(removeDuplicateActiveRuleParamsSql)
+      .execute()
+      .commit();
+    context.prepareUpsert(removeDuplicateRulesParamsSql)
+      .execute()
+      .commit();
+  }
+}
diff --git a/server/sonar-db-migration/src/test/java/org/sonar/server/platform/db/migration/version/v72/AddUniqueIndexOnRulesParametersTest.java b/server/sonar-db-migration/src/test/java/org/sonar/server/platform/db/migration/version/v72/AddUniqueIndexOnRulesParametersTest.java
new file mode 100644
index 00000000000..0f7cddf7a7c
--- /dev/null
+++ b/server/sonar-db-migration/src/test/java/org/sonar/server/platform/db/migration/version/v72/AddUniqueIndexOnRulesParametersTest.java
@@ -0,0 +1,53 @@
+/*
+ * SonarQube
+ * Copyright (C) 2009-2018 SonarSource SA
+ * mailto:info AT sonarsource DOT com
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 3 of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ */
+package org.sonar.server.platform.db.migration.version.v72;
+
+import java.sql.SQLException;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.ExpectedException;
+import org.sonar.db.CoreDbTester;
+
+public class AddUniqueIndexOnRulesParametersTest {
+
+  @Rule
+  public final CoreDbTester db = CoreDbTester.createForSchema(AddUniqueIndexOnRulesParametersTest.class, "rules_parameters.sql");
+
+  @Rule
+  public ExpectedException expectedException = ExpectedException.none();
+
+  private AddUniqueIndexOnRulesParameters underTest = new AddUniqueIndexOnRulesParameters(db.database());
+
+  @Test
+  public void add_unique_indexes() throws SQLException {
+    underTest.execute();
+
+    db.assertUniqueIndex("rules_parameters", "rules_parameters_unique", "rule_id", "name");
+  }
+
+  @Test
+  public void migration_is_not_reentrant() throws SQLException {
+    underTest.execute();
+
+    expectedException.expect(IllegalStateException.class);
+
+    underTest.execute();
+  }
+}
diff --git a/server/sonar-db-migration/src/test/java/org/sonar/server/platform/db/migration/version/v72/DbVersion72Test.java b/server/sonar-db-migration/src/test/java/org/sonar/server/platform/db/migration/version/v72/DbVersion72Test.java
index 8c25fd54a3b..5278c13d921 100644
--- a/server/sonar-db-migration/src/test/java/org/sonar/server/platform/db/migration/version/v72/DbVersion72Test.java
+++ b/server/sonar-db-migration/src/test/java/org/sonar/server/platform/db/migration/version/v72/DbVersion72Test.java
@@ -34,7 +34,7 @@ public class DbVersion72Test {
 
   @Test
   public void verify_migration_count() {
-    verifyMigrationCount(underTest, 28);
+    verifyMigrationCount(underTest, 30);
   }
 
 }
diff --git a/server/sonar-db-migration/src/test/java/org/sonar/server/platform/db/migration/version/v72/PurgeDuplicateRulesParametersTest.java b/server/sonar-db-migration/src/test/java/org/sonar/server/platform/db/migration/version/v72/PurgeDuplicateRulesParametersTest.java
new file mode 100644
index 00000000000..782bf869856
--- /dev/null
+++ b/server/sonar-db-migration/src/test/java/org/sonar/server/platform/db/migration/version/v72/PurgeDuplicateRulesParametersTest.java
@@ -0,0 +1,92 @@
+/*
+ * SonarQube
+ * Copyright (C) 2009-2018 SonarSource SA
+ * mailto:info AT sonarsource DOT com
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 3 of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ */
+package org.sonar.server.platform.db.migration.version.v72;
+
+import com.google.common.collect.ImmutableMap;
+import java.sql.SQLException;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.ExpectedException;
+import org.sonar.db.CoreDbTester;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+public class PurgeDuplicateRulesParametersTest {
+
+  @Rule
+  public final CoreDbTester db = CoreDbTester.createForSchema(PurgeDuplicateRulesParametersTest.class, "rules_parameters_etc.sql");
+
+  @Rule
+  public ExpectedException expectedException = ExpectedException.none();
+
+  private PurgeDuplicateRulesParameters underTest = new PurgeDuplicateRulesParameters(db.database());
+
+  @Test
+  public void migration_deletes_duplicates() throws SQLException {
+    ImmutableMap<String, Object> dup = ImmutableMap.<String, Object>builder()
+      .put("rule_id", 10)
+      .put("name", "foo")
+      .put("param_type", "INTEGER")
+      .build();
+    db.executeInsert("rules_parameters", dup);
+    Object id1 = lastId();
+
+    db.executeInsert("rules_parameters", dup);
+    Object id2 = lastId();
+
+    db.executeInsert("rules_parameters", ImmutableMap.<String, Object>builder()
+      .put("rule_id", 20)
+      .put("name", "bar")
+      .put("param_type", "INTEGER")
+      .build());
+    Object id3 = lastId();
+
+    db.executeInsert("active_rule_parameters", ImmutableMap.<String, Object>builder()
+      .put("active_rule_id", 1)
+      .put("rules_parameter_id", id1)
+      .put("rules_parameter_key", "foo")
+      .build());
+    db.executeInsert("active_rule_parameters", ImmutableMap.<String, Object>builder()
+      .put("active_rule_id", 2)
+      .put("rules_parameter_id", id2)
+      .put("rules_parameter_key", "foo")
+      .build());
+    db.executeInsert("active_rule_parameters", ImmutableMap.<String, Object>builder()
+      .put("active_rule_id", 3)
+      .put("rules_parameter_id", id3)
+      .put("rules_parameter_key", "bar")
+      .build());
+
+    assertThat(db.countRowsOfTable("rules_parameters")).isEqualTo(3);
+    assertThat(db.countRowsOfTable("active_rule_parameters")).isEqualTo(3);
+
+    underTest.execute();
+
+    assertThat(db.countRowsOfTable("rules_parameters")).isEqualTo(2);
+    assertThat(db.countRowsOfTable("active_rule_parameters")).isEqualTo(2);
+    assertThat(db.countSql("select count(*) from rules_parameters where id = " + id2)).isEqualTo(0);
+    assertThat(db.countSql("select count(*) from active_rule_parameters where rules_parameter_id = " + id2)).isEqualTo(0);
+  }
+
+  private long lastId() {
+    return (Long) db.selectFirst("select max(id) id from rules_parameters").get("ID");
+  }
+
+}
diff --git a/server/sonar-db-migration/src/test/resources/org/sonar/server/platform/db/migration/version/v72/AddUniqueIndexOnRulesParametersTest/rules_parameters.sql b/server/sonar-db-migration/src/test/resources/org/sonar/server/platform/db/migration/version/v72/AddUniqueIndexOnRulesParametersTest/rules_parameters.sql
new file mode 100644
index 00000000000..26b436bb1f9
--- /dev/null
+++ b/server/sonar-db-migration/src/test/resources/org/sonar/server/platform/db/migration/version/v72/AddUniqueIndexOnRulesParametersTest/rules_parameters.sql
@@ -0,0 +1,9 @@
+CREATE TABLE "RULES_PARAMETERS" (
+  "ID" INTEGER NOT NULL GENERATED BY DEFAULT AS IDENTITY (START WITH 1, INCREMENT BY 1),
+  "RULE_ID" INTEGER NOT NULL,
+  "NAME" VARCHAR(128) NOT NULL,
+  "PARAM_TYPE" VARCHAR(512) NOT NULL,
+  "DEFAULT_VALUE" VARCHAR(4000),
+  "DESCRIPTION" VARCHAR(4000)
+);
+CREATE INDEX "RULES_PARAMETERS_RULE_ID" ON "RULES_PARAMETERS" ("RULE_ID");
diff --git a/server/sonar-db-migration/src/test/resources/org/sonar/server/platform/db/migration/version/v72/PurgeDuplicateRulesParametersTest/rules_parameters_etc.sql b/server/sonar-db-migration/src/test/resources/org/sonar/server/platform/db/migration/version/v72/PurgeDuplicateRulesParametersTest/rules_parameters_etc.sql
new file mode 100644
index 00000000000..3ceb8ea5762
--- /dev/null
+++ b/server/sonar-db-migration/src/test/resources/org/sonar/server/platform/db/migration/version/v72/PurgeDuplicateRulesParametersTest/rules_parameters_etc.sql
@@ -0,0 +1,18 @@
+CREATE TABLE "RULES_PARAMETERS" (
+  "ID" INTEGER NOT NULL GENERATED BY DEFAULT AS IDENTITY (START WITH 1, INCREMENT BY 1),
+  "RULE_ID" INTEGER NOT NULL,
+  "NAME" VARCHAR(128) NOT NULL,
+  "PARAM_TYPE" VARCHAR(512) NOT NULL,
+  "DEFAULT_VALUE" VARCHAR(4000),
+  "DESCRIPTION" VARCHAR(4000)
+);
+CREATE INDEX "RULES_PARAMETERS_RULE_ID" ON "RULES_PARAMETERS" ("RULE_ID");
+
+CREATE TABLE "ACTIVE_RULE_PARAMETERS" (
+  "ID" INTEGER NOT NULL GENERATED BY DEFAULT AS IDENTITY (START WITH 1, INCREMENT BY 1),
+  "ACTIVE_RULE_ID" INTEGER NOT NULL,
+  "RULES_PARAMETER_ID" INTEGER NOT NULL,
+  "RULES_PARAMETER_KEY" VARCHAR(128),
+  "VALUE" VARCHAR(4000)
+);
+CREATE INDEX "IX_ARP_ON_ACTIVE_RULE_ID" ON "ACTIVE_RULE_PARAMETERS" ("ACTIVE_RULE_ID");
diff --git a/server/sonar-server/src/test/java/org/sonar/server/qualityprofile/ws/CompareActionTest.java b/server/sonar-server/src/test/java/org/sonar/server/qualityprofile/ws/CompareActionTest.java
index e08f9ac7bd6..0c64765e101 100644
--- a/server/sonar-server/src/test/java/org/sonar/server/qualityprofile/ws/CompareActionTest.java
+++ b/server/sonar-server/src/test/java/org/sonar/server/qualityprofile/ws/CompareActionTest.java
@@ -208,8 +208,6 @@ public class CompareActionTest {
       .setStatus(RuleStatus.READY);
     RuleDefinitionDto ruleDefinition = rule.getDefinition();
     db.ruleDao().insert(session, ruleDefinition);
-    RuleParamDto param = RuleParamDto.createFor(ruleDefinition).setName("param_" + id).setType(RuleParamType.STRING.toString());
-    db.ruleDao().insertRuleParam(session, ruleDefinition, param);
     return ruleDefinition;
   }