From: Janos Gyerik Date: Mon, 11 Jun 2018 11:59:33 +0000 (+0200) Subject: SONAR-10663 Prevent duplicate rule parameters in the database (#341) X-Git-Tag: 7.5~1051 X-Git-Url: https://source.dussan.org/?a=commitdiff_plain;h=3c1453a726a753e9f8e15a43d70d7e7ee1d835a8;p=sonarqube.git SONAR-10663 Prevent duplicate rule parameters in the database (#341) --- 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 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 dup = ImmutableMap.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.builder() + .put("rule_id", 20) + .put("name", "bar") + .put("param_type", "INTEGER") + .build()); + Object id3 = lastId(); + + db.executeInsert("active_rule_parameters", ImmutableMap.builder() + .put("active_rule_id", 1) + .put("rules_parameter_id", id1) + .put("rules_parameter_key", "foo") + .build()); + db.executeInsert("active_rule_parameters", ImmutableMap.builder() + .put("active_rule_id", 2) + .put("rules_parameter_id", id2) + .put("rules_parameter_key", "foo") + .build()); + db.executeInsert("active_rule_parameters", ImmutableMap.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; }