From: Julien Lancelot Date: Fri, 4 Apr 2014 12:14:21 +0000 (+0200) Subject: Fix quality flaws X-Git-Tag: 4.3~130 X-Git-Url: https://source.dussan.org/?a=commitdiff_plain;h=41c86d81b9c4ae4f045da26a631e38c201521077;p=sonarqube.git Fix quality flaws --- diff --git a/sonar-core/src/test/java/org/sonar/core/component/ComponentDtoTest.java b/sonar-core/src/test/java/org/sonar/core/component/ComponentDtoTest.java new file mode 100644 index 00000000000..1b2ea6a641f --- /dev/null +++ b/sonar-core/src/test/java/org/sonar/core/component/ComponentDtoTest.java @@ -0,0 +1,65 @@ +/* + * SonarQube, open source software quality management tool. + * Copyright (C) 2008-2014 SonarSource + * mailto:contact AT sonarsource DOT com + * + * SonarQube 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. + * + * SonarQube 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.core.component; + +import org.junit.Test; + +import static org.fest.assertions.Assertions.assertThat; + +public class ComponentDtoTest { + + @Test + public void setters_and_getters() throws Exception { + ComponentDto componentDto = new ComponentDto() + .setId(1L) + .setKey("org.struts:struts-core:src/org/struts/RequestContext.java") + .setName("RequestContext.java") + .setLongName("org.struts.RequestContext") + .setQualifier("FIL") + .setPath("src/org/struts/RequestContext.java") + .setProjectId(2L) + .setSubProjectId(3L); + + assertThat(componentDto.getId()).isEqualTo(1L); + assertThat(componentDto.key()).isEqualTo("org.struts:struts-core:src/org/struts/RequestContext.java"); + assertThat(componentDto.name()).isEqualTo("RequestContext.java"); + assertThat(componentDto.longName()).isEqualTo("org.struts.RequestContext"); + assertThat(componentDto.qualifier()).isEqualTo("FIL"); + assertThat(componentDto.path()).isEqualTo("src/org/struts/RequestContext.java"); + assertThat(componentDto.projectId()).isEqualTo(2L); + assertThat(componentDto.subProjectId()).isEqualTo(3L); + } + + @Test + public void equals_and_hashcode() throws Exception { + ComponentDto dto = new ComponentDto().setId(1L); + ComponentDto dtoWithSameId = new ComponentDto().setId(1L); + ComponentDto dtoWithDifferentId = new ComponentDto().setId(2L); + + assertThat(dto).isEqualTo(dto); + assertThat(dto).isEqualTo(dtoWithSameId); + assertThat(dto).isNotEqualTo(dtoWithDifferentId); + + assertThat(dto.hashCode()).isEqualTo(dto.hashCode()); + assertThat(dto.hashCode()).isEqualTo(dtoWithSameId.hashCode()); + assertThat(dto.hashCode()).isNotEqualTo(dtoWithDifferentId.hashCode()); + } +} diff --git a/sonar-plugin-api/src/main/java/org/sonar/api/technicaldebt/batch/internal/DefaultCharacteristic.java b/sonar-plugin-api/src/main/java/org/sonar/api/technicaldebt/batch/internal/DefaultCharacteristic.java index e5de8f01f4a..1f0beba8d2c 100644 --- a/sonar-plugin-api/src/main/java/org/sonar/api/technicaldebt/batch/internal/DefaultCharacteristic.java +++ b/sonar-plugin-api/src/main/java/org/sonar/api/technicaldebt/batch/internal/DefaultCharacteristic.java @@ -83,7 +83,6 @@ public class DefaultCharacteristic implements Characteristic { return this; } - // TODO Replace this is a TechnicalDebtFactory class public DefaultCharacteristic setName(String s, boolean asKey) { this.name = StringUtils.trimToNull(s); if (asKey) { @@ -157,11 +156,12 @@ public class DefaultCharacteristic implements Characteristic { return this; } + @CheckForNull public Date updatedAt() { return updatedAt; } - public DefaultCharacteristic setUpdatedAt(Date updatedAt) { + public DefaultCharacteristic setUpdatedAt(@Nullable Date updatedAt) { this.updatedAt = updatedAt; return this; } diff --git a/sonar-plugin-api/src/test/java/org/sonar/api/batch/debt/internal/DefaultDebtCharacteristicTest.java b/sonar-plugin-api/src/test/java/org/sonar/api/batch/debt/internal/DefaultDebtCharacteristicTest.java new file mode 100644 index 00000000000..2a09fe5bef9 --- /dev/null +++ b/sonar-plugin-api/src/test/java/org/sonar/api/batch/debt/internal/DefaultDebtCharacteristicTest.java @@ -0,0 +1,81 @@ +/* + * SonarQube, open source software quality management tool. + * Copyright (C) 2008-2014 SonarSource + * mailto:contact AT sonarsource DOT com + * + * SonarQube 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. + * + * SonarQube 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.api.batch.debt.internal; + +import org.junit.Test; + +import java.util.Date; + +import static org.fest.assertions.Assertions.assertThat; + +public class DefaultDebtCharacteristicTest { + + @Test + public void setter_and_getter_on_characteristic() throws Exception { + DefaultDebtCharacteristic debtCharacteristic = new DefaultDebtCharacteristic() + .setId(1) + .setKey("PORTABILITY") + .setName("Portability") + .setOrder(1) + .setCreatedAt(new Date()) + .setUpdatedAt(new Date()); + + assertThat(debtCharacteristic.id()).isEqualTo(1); + assertThat(debtCharacteristic.key()).isEqualTo("PORTABILITY"); + assertThat(debtCharacteristic.name()).isEqualTo("Portability"); + assertThat(debtCharacteristic.order()).isEqualTo(1); + assertThat(debtCharacteristic.parentId()).isNull(); + assertThat(debtCharacteristic.isSub()).isFalse(); + assertThat(debtCharacteristic.createdAt()).isNotNull(); + assertThat(debtCharacteristic.updatedAt()).isNotNull(); + } + + @Test + public void setter_and_getter_on_sub_characteristic() throws Exception { + DefaultDebtCharacteristic debtCharacteristic = new DefaultDebtCharacteristic() + .setId(1) + .setKey("COMPILER") + .setName("Compiler") + .setParentId(2) + .setCreatedAt(new Date()) + .setUpdatedAt(new Date()); + + assertThat(debtCharacteristic.id()).isEqualTo(1); + assertThat(debtCharacteristic.key()).isEqualTo("COMPILER"); + assertThat(debtCharacteristic.name()).isEqualTo("Compiler"); + assertThat(debtCharacteristic.order()).isNull(); + assertThat(debtCharacteristic.parentId()).isEqualTo(2); + assertThat(debtCharacteristic.isSub()).isTrue(); + assertThat(debtCharacteristic.createdAt()).isNotNull(); + assertThat(debtCharacteristic.updatedAt()).isNotNull(); + } + + @Test + public void to_string() throws Exception { + assertThat(new DefaultDebtCharacteristic() + .setId(1) + .setKey("PORTABILITY") + .setName("Portability") + .setOrder(1) + .setCreatedAt(new Date()) + .setUpdatedAt(new Date())).isNotNull(); + } +} diff --git a/sonar-plugin-api/src/test/java/org/sonar/api/batch/debt/internal/DefaultDebtModelTest.java b/sonar-plugin-api/src/test/java/org/sonar/api/batch/debt/internal/DefaultDebtModelTest.java index 561cb5ed3ae..a5dfe422bf2 100644 --- a/sonar-plugin-api/src/test/java/org/sonar/api/batch/debt/internal/DefaultDebtModelTest.java +++ b/sonar-plugin-api/src/test/java/org/sonar/api/batch/debt/internal/DefaultDebtModelTest.java @@ -72,6 +72,9 @@ public class DefaultDebtModelTest { assertThat(debtCharacteristic.order()).isEqualTo(1); assertThat(debtCharacteristic.parentId()).isNull(); assertThat(debtCharacteristic.isSub()).isFalse(); + + + assertThat(debtModel.characteristicById(555)).isNull(); } @Test @@ -84,5 +87,7 @@ public class DefaultDebtModelTest { assertThat(debtCharacteristic.order()).isNull(); assertThat(debtCharacteristic.parentId()).isEqualTo(1); assertThat(debtCharacteristic.isSub()).isTrue(); + + assertThat(debtModel.characteristicByKey("UNKNOWN")).isNull(); } } diff --git a/sonar-plugin-api/src/test/java/org/sonar/api/server/debt/internal/DefaultDebtCharacteristicTest.java b/sonar-plugin-api/src/test/java/org/sonar/api/server/debt/internal/DefaultDebtCharacteristicTest.java new file mode 100644 index 00000000000..f7d84cef57e --- /dev/null +++ b/sonar-plugin-api/src/test/java/org/sonar/api/server/debt/internal/DefaultDebtCharacteristicTest.java @@ -0,0 +1,81 @@ +/* + * SonarQube, open source software quality management tool. + * Copyright (C) 2008-2014 SonarSource + * mailto:contact AT sonarsource DOT com + * + * SonarQube 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. + * + * SonarQube 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.api.server.debt.internal; + +import org.junit.Test; + +import java.util.Date; + +import static org.fest.assertions.Assertions.assertThat; + +public class DefaultDebtCharacteristicTest { + + @Test + public void setter_and_getter_on_characteristic() throws Exception { + DefaultDebtCharacteristic debtCharacteristic = new DefaultDebtCharacteristic() + .setId(1) + .setKey("PORTABILITY") + .setName("Portability") + .setOrder(1) + .setCreatedAt(new Date()) + .setUpdatedAt(new Date()); + + assertThat(debtCharacteristic.id()).isEqualTo(1); + assertThat(debtCharacteristic.key()).isEqualTo("PORTABILITY"); + assertThat(debtCharacteristic.name()).isEqualTo("Portability"); + assertThat(debtCharacteristic.order()).isEqualTo(1); + assertThat(debtCharacteristic.parentId()).isNull(); + assertThat(debtCharacteristic.isSub()).isFalse(); + assertThat(debtCharacteristic.createdAt()).isNotNull(); + assertThat(debtCharacteristic.updatedAt()).isNotNull(); + } + + @Test + public void setter_and_getter_on_sub_characteristic() throws Exception { + DefaultDebtCharacteristic debtCharacteristic = new DefaultDebtCharacteristic() + .setId(1) + .setKey("COMPILER") + .setName("Compiler") + .setParentId(2) + .setCreatedAt(new Date()) + .setUpdatedAt(new Date()); + + assertThat(debtCharacteristic.id()).isEqualTo(1); + assertThat(debtCharacteristic.key()).isEqualTo("COMPILER"); + assertThat(debtCharacteristic.name()).isEqualTo("Compiler"); + assertThat(debtCharacteristic.order()).isNull(); + assertThat(debtCharacteristic.parentId()).isEqualTo(2); + assertThat(debtCharacteristic.isSub()).isTrue(); + assertThat(debtCharacteristic.createdAt()).isNotNull(); + assertThat(debtCharacteristic.updatedAt()).isNotNull(); + } + + @Test + public void to_string() throws Exception { + assertThat(new DefaultDebtCharacteristic() + .setId(1) + .setKey("PORTABILITY") + .setName("Portability") + .setOrder(1) + .setCreatedAt(new Date()) + .setUpdatedAt(new Date())).isNotNull(); + } +} diff --git a/sonar-server/src/main/java/org/sonar/server/debt/DebtCharacteristicsXMLImporter.java b/sonar-server/src/main/java/org/sonar/server/debt/DebtCharacteristicsXMLImporter.java index d0855fe34d8..3ff96509ce8 100644 --- a/sonar-server/src/main/java/org/sonar/server/debt/DebtCharacteristicsXMLImporter.java +++ b/sonar-server/src/main/java/org/sonar/server/debt/DebtCharacteristicsXMLImporter.java @@ -85,7 +85,6 @@ public class DebtCharacteristicsXMLImporter implements ServerComponent { while (cursor.getNext() != null) { String node = cursor.getLocalName(); if (StringUtils.equals(node, CHARACTERISTIC_KEY)) { - // TODO Attached to parent only if a key is existing, otherwise characteristic with empty key can be added. characteristic.setKey(cursor.collectDescendantText().trim()); if (parent == null) { characteristic.setOrder(debtModel.rootCharacteristics().size() + 1); diff --git a/sonar-server/src/main/java/org/sonar/server/rule/RegisterRules.java b/sonar-server/src/main/java/org/sonar/server/rule/RegisterRules.java index 89db8d21544..e320369efdf 100644 --- a/sonar-server/src/main/java/org/sonar/server/rule/RegisterRules.java +++ b/sonar-server/src/main/java/org/sonar/server/rule/RegisterRules.java @@ -268,27 +268,31 @@ public class RegisterRules implements Startable { } private boolean mergeDebtDefinitions(RulesDefinition.Rule def, RuleDto dto, @Nullable CharacteristicDto subCharacteristic) { - boolean changed = false; + // Debt definitions are set to null if the sub-characteristic and the remediation function are null + DebtRemediationFunction debtRemediationFunction = subCharacteristic != null ? def.debtRemediationFunction() : null; + boolean hasDebt = subCharacteristic != null && debtRemediationFunction != null; + return mergeDebtDefinitions(def, dto, + hasDebt ? subCharacteristic.getId() : null, + debtRemediationFunction != null ? debtRemediationFunction.type().name() : null, + hasDebt ? debtRemediationFunction.coefficient() : null, + hasDebt ? debtRemediationFunction.offset() : null, + hasDebt ? def.effortToFixDescription() : null); + } - // Debt definitions are set to null if the sub-characteristic is null or unknown - boolean hasCharacteristic = subCharacteristic != null; - DebtRemediationFunction debtRemediationFunction = hasCharacteristic ? def.debtRemediationFunction() : null; - Integer characteristicId = hasCharacteristic ? subCharacteristic.getId() : null; - String remediationFactor = hasCharacteristic ? debtRemediationFunction.coefficient() : null; - String remediationOffset = hasCharacteristic ? debtRemediationFunction.offset() : null; - String effortToFixDescription = hasCharacteristic ? def.effortToFixDescription() : null; + private boolean mergeDebtDefinitions(RulesDefinition.Rule def, RuleDto dto,@Nullable Integer characteristicId, @Nullable String remediationFunction, + @Nullable String remediationCoefficient, @Nullable String remediationOffset, @Nullable String effortToFixDescription) { + boolean changed = false; if (!ObjectUtils.equals(dto.getDefaultSubCharacteristicId(), characteristicId)) { dto.setDefaultSubCharacteristicId(characteristicId); changed = true; } - String remediationFunctionString = debtRemediationFunction != null ? debtRemediationFunction.type().name() : null; - if (!StringUtils.equals(dto.getDefaultRemediationFunction(), remediationFunctionString)) { - dto.setDefaultRemediationFunction(remediationFunctionString); + if (!StringUtils.equals(dto.getDefaultRemediationFunction(), remediationFunction)) { + dto.setDefaultRemediationFunction(remediationFunction); changed = true; } - if (!StringUtils.equals(dto.getDefaultRemediationCoefficient(), remediationFactor)) { - dto.setDefaultRemediationCoefficient(remediationFactor); + if (!StringUtils.equals(dto.getDefaultRemediationCoefficient(), remediationCoefficient)) { + dto.setDefaultRemediationCoefficient(remediationCoefficient); changed = true; } if (!StringUtils.equals(dto.getDefaultRemediationOffset(), remediationOffset)) { diff --git a/sonar-server/src/main/java/org/sonar/server/rule/Rule.java b/sonar-server/src/main/java/org/sonar/server/rule/Rule.java index 6bfab78a530..ba5c933524c 100644 --- a/sonar-server/src/main/java/org/sonar/server/rule/Rule.java +++ b/sonar-server/src/main/java/org/sonar/server/rule/Rule.java @@ -180,9 +180,7 @@ public class Rule { private Collection adminTags; private Collection params; private String debtCharacteristicKey; - private String debtCharacteristicName; private String debtSubCharacteristicKey; - private String debtSubCharacteristicName; private DebtRemediationFunction debtRemediationFunction; private Date createdAt; private Date updatedAt; diff --git a/sonar-server/src/main/java/org/sonar/server/rule/RuleDocumentParser.java b/sonar-server/src/main/java/org/sonar/server/rule/RuleDocumentParser.java index 09c08cb2d65..ac6c87e3c95 100644 --- a/sonar-server/src/main/java/org/sonar/server/rule/RuleDocumentParser.java +++ b/sonar-server/src/main/java/org/sonar/server/rule/RuleDocumentParser.java @@ -54,22 +54,28 @@ public class RuleDocumentParser { .setCreatedAt(parseOptionalDate(RuleDocument.FIELD_CREATED_AT, ruleSource)) .setUpdatedAt(parseOptionalDate(RuleDocument.FIELD_UPDATED_AT, ruleSource)); + addRuleDebt(ruleSource, ruleBuilder); + addRuleNote(ruleSource, ruleBuilder); + addRuleParams(ruleSource, ruleBuilder); + addRuleTags(ruleSource, ruleBuilder); + return ruleBuilder.build(); + } + + private static void addRuleDebt(Map ruleSource, Rule.Builder ruleBuilder) { if (ruleSource.containsKey(RuleDocument.FIELD_CHARACTERISTIC_KEY)) { - try { - ruleBuilder - .setDebtCharacteristicKey((String) ruleSource.get(RuleDocument.FIELD_CHARACTERISTIC_KEY)) - .setDebtSubCharacteristicKey((String) ruleSource.get(RuleDocument.FIELD_SUB_CHARACTERISTIC_KEY)) - .setDebtRemediationFunction( - new DefaultDebtRemediationFunction(DebtRemediationFunction.Type.valueOf((String) ruleSource.get(RuleDocument.FIELD_REMEDIATION_FUNCTION)), - (String) ruleSource.get(RuleDocument.FIELD_REMEDIATION_COEFFICIENT), - (String) ruleSource.get(RuleDocument.FIELD_REMEDIATION_OFFSET)) - ) - ; - } catch (IllegalArgumentException e) { - throw new RuntimeException("Fail on rule '" + ruleSource.get(RuleDocument.FIELD_KEY) + "', " + e.getMessage(), e); - } + ruleBuilder + .setDebtCharacteristicKey((String) ruleSource.get(RuleDocument.FIELD_CHARACTERISTIC_KEY)) + .setDebtSubCharacteristicKey((String) ruleSource.get(RuleDocument.FIELD_SUB_CHARACTERISTIC_KEY)) + .setDebtRemediationFunction( + new DefaultDebtRemediationFunction(DebtRemediationFunction.Type.valueOf((String) ruleSource.get(RuleDocument.FIELD_REMEDIATION_FUNCTION)), + (String) ruleSource.get(RuleDocument.FIELD_REMEDIATION_COEFFICIENT), + (String) ruleSource.get(RuleDocument.FIELD_REMEDIATION_OFFSET)) + ) + ; } + } + private static void addRuleNote(Map ruleSource, Rule.Builder ruleBuilder) { if (ruleSource.containsKey(RuleDocument.FIELD_NOTE)) { Map ruleNoteDocument = (Map) ruleSource.get(RuleDocument.FIELD_NOTE); ruleBuilder.setRuleNote(new RuleNote( @@ -79,7 +85,9 @@ public class RuleDocumentParser { parseOptionalDate(RuleDocument.FIELD_NOTE_UPDATED_AT, ruleNoteDocument) )); } + } + private static void addRuleParams(Map ruleSource, Rule.Builder ruleBuilder) { List params = Lists.newArrayList(); if (ruleSource.containsKey(RuleDocument.FIELD_PARAMS)) { Map> ruleParams = Maps.newHashMap(); @@ -97,7 +105,9 @@ public class RuleDocumentParser { } } ruleBuilder.setParams(params); + } + private static void addRuleTags(Map ruleSource, Rule.Builder ruleBuilder) { List systemTags = newArrayList(); if (ruleSource.containsKey(RuleDocument.FIELD_SYSTEM_TAGS)) { for (String tag : (List) ruleSource.get(RuleDocument.FIELD_SYSTEM_TAGS)) { @@ -113,8 +123,6 @@ public class RuleDocumentParser { } } ruleBuilder.setAdminTags(adminTags); - - return ruleBuilder.build(); } public static Date parseOptionalDate(String field, Map ruleSource) { diff --git a/sonar-server/src/main/java/org/sonar/server/rule/RuleOperations.java b/sonar-server/src/main/java/org/sonar/server/rule/RuleOperations.java index 75c4b3c26e3..be2b3022cce 100644 --- a/sonar-server/src/main/java/org/sonar/server/rule/RuleOperations.java +++ b/sonar-server/src/main/java/org/sonar/server/rule/RuleOperations.java @@ -287,20 +287,13 @@ public class RuleOperations implements ServerComponent { } public boolean updateRule(RuleDto ruleDto, @Nullable CharacteristicDto newSubCharacteristic, @Nullable String newFunction, - @Nullable String newCoefficient, @Nullable String newOffset, Date updateDate, SqlSession session){ + @Nullable String newCoefficient, @Nullable String newOffset, Date updateDate, SqlSession session) { boolean needUpdate = false; - // A sub-characteristic is given -> update rule debt - if (newSubCharacteristic != null) { - boolean iSameAsDefaultValues = newSubCharacteristic.getId().equals(ruleDto.getDefaultSubCharacteristicId()) - && isSameRemediationFunction(newFunction, newCoefficient, newOffset, - ruleDto.getDefaultRemediationFunction(), ruleDto.getDefaultRemediationCoefficient(), ruleDto.getDefaultRemediationOffset()); - boolean iSameAsOverriddenValues = newSubCharacteristic.getId().equals(ruleDto.getSubCharacteristicId()) - && isSameRemediationFunction(newFunction, newCoefficient, newOffset, - ruleDto.getRemediationFunction(), ruleDto.getRemediationCoefficient(), ruleDto.getRemediationOffset()); - + // A sub-characteristic and a remediation function is given -> update rule debt + if (newSubCharacteristic != null && newFunction != null) { // New values are the same as the default values -> set overridden values to null - if (iSameAsDefaultValues) { + if (isRuleDebtSameAsDefaultValues(ruleDto, newSubCharacteristic, newFunction, newCoefficient, newOffset)) { ruleDto.setSubCharacteristicId(null); ruleDto.setRemediationFunction(null); ruleDto.setRemediationCoefficient(null); @@ -308,7 +301,7 @@ public class RuleOperations implements ServerComponent { needUpdate = true; // New values are not the same as the overridden values -> update overridden values with new values - } else if (!iSameAsOverriddenValues) { + } else if (!isRuleDebtSameAsOverriddenValues(ruleDto, newSubCharacteristic, newFunction, newCoefficient, newOffset)) { ruleDto.setSubCharacteristicId(newSubCharacteristic.getId()); DefaultDebtRemediationFunction debtRemediationFunction = new DefaultDebtRemediationFunction(DebtRemediationFunction.Type.valueOf(newFunction), newCoefficient, newOffset); @@ -338,6 +331,19 @@ public class RuleOperations implements ServerComponent { return needUpdate; } + private static boolean isRuleDebtSameAsDefaultValues(RuleDto ruleDto, CharacteristicDto newSubCharacteristic, @Nullable String newFunction, + @Nullable String newCoefficient, @Nullable String newOffset) { + return newSubCharacteristic.getId().equals(ruleDto.getDefaultSubCharacteristicId()) && + isSameRemediationFunction(newFunction, newCoefficient, newOffset, ruleDto.getDefaultRemediationFunction(), ruleDto.getDefaultRemediationCoefficient(), + ruleDto.getDefaultRemediationOffset()); + } + + private static boolean isRuleDebtSameAsOverriddenValues(RuleDto ruleDto, CharacteristicDto newSubCharacteristic, @Nullable String newFunction, + @Nullable String newCoefficient, @Nullable String newOffset) { + return newSubCharacteristic.getId().equals(ruleDto.getSubCharacteristicId()) + && isSameRemediationFunction(newFunction, newCoefficient, newOffset, ruleDto.getRemediationFunction(), ruleDto.getRemediationCoefficient(), ruleDto.getRemediationOffset()); + } + private static boolean isSameRemediationFunction(@Nullable String newFunction, @Nullable String newCoefficient, @Nullable String newOffset, String oldFunction, @Nullable String oldCoefficient, @Nullable String oldOffset) { return new EqualsBuilder() diff --git a/sonar-server/src/main/java/org/sonar/server/rule/RuleRegistry.java b/sonar-server/src/main/java/org/sonar/server/rule/RuleRegistry.java index 5258139b56d..6bc01497bd5 100644 --- a/sonar-server/src/main/java/org/sonar/server/rule/RuleRegistry.java +++ b/sonar-server/src/main/java/org/sonar/server/rule/RuleRegistry.java @@ -176,7 +176,9 @@ public class RuleRegistry { Integer ruleId = rule.getId(); Integer effectiveSubCharacteristicId = rule.getSubCharacteristicId() != null ? rule.getSubCharacteristicId() : rule.getDefaultSubCharacteristicId(); CharacteristicDto subCharacteristic = effectiveSubCharacteristicId != null ? characteristicDao.selectById(effectiveSubCharacteristicId, session) : null; - CharacteristicDto characteristic = subCharacteristic != null ? characteristicDao.selectById(subCharacteristic.getParentId(), session) : null; + // The parent id of the sub-characteristic should never be null + CharacteristicDto characteristic = (subCharacteristic != null && subCharacteristic.getParentId() != null) ? + characteristicDao.selectById(subCharacteristic.getParentId(), session) : null; searchIndex.putSynchronous(INDEX_RULES, TYPE_RULE, Long.toString(ruleId), ruleDocument(rule, characteristic, subCharacteristic, ruleDao.selectParametersByRuleIds(newArrayList(ruleId), session), @@ -240,6 +242,16 @@ public class RuleRegistry { .field(RuleDocument.FIELD_CARDINALITY, rule.getCardinality()) .field(RuleDocument.FIELD_CREATED_AT, rule.getCreatedAt()) .field(RuleDocument.FIELD_UPDATED_AT, rule.getUpdatedAt()); + addRuleDebt(document, rule, characteristicDto, subCharacteristicDto); + addRuleNote(document, rule); + addRuleParams(document, rule, params); + addRuleTags(document, rule, tags); + document.endObject(); + return document; + } + + private void addRuleDebt(XContentBuilder document, RuleDto rule, @Nullable CharacteristicDto characteristicDto, @Nullable CharacteristicDto subCharacteristicDto) + throws IOException { if (characteristicDto != null && subCharacteristicDto != null) { boolean isFunctionOverridden = rule.getRemediationFunction() != null; document @@ -251,7 +263,9 @@ public class RuleRegistry { .field(RuleDocument.FIELD_REMEDIATION_COEFFICIENT, isFunctionOverridden ? rule.getRemediationCoefficient() : rule.getDefaultRemediationCoefficient()) .field(RuleDocument.FIELD_REMEDIATION_OFFSET, isFunctionOverridden ? rule.getRemediationOffset() : rule.getDefaultRemediationOffset()); } + } + private void addRuleNote(XContentBuilder document, RuleDto rule) throws IOException { if (rule.getNoteData() != null || rule.getNoteUserLogin() != null) { document.startObject(RuleDocument.FIELD_NOTE) .field(RuleDocument.FIELD_NOTE_DATA, rule.getNoteData()) @@ -260,6 +274,9 @@ public class RuleRegistry { .field(RuleDocument.FIELD_NOTE_UPDATED_AT, rule.getNoteUpdatedAt()) .endObject(); } + } + + private void addRuleParams(XContentBuilder document, RuleDto rule, Collection params) throws IOException { if (!params.isEmpty()) { document.startArray(RuleDocument.FIELD_PARAMS); for (RuleParamDto param : params) { @@ -272,6 +289,9 @@ public class RuleRegistry { } document.endArray(); } + } + + private void addRuleTags(XContentBuilder document, RuleDto rule, Collection tags) throws IOException { List systemTags = Lists.newArrayList(); List adminTags = Lists.newArrayList(); for (RuleRuleTagDto tag : tags) { @@ -287,9 +307,6 @@ public class RuleRegistry { if (!adminTags.isEmpty()) { document.array(RuleDocument.FIELD_ADMIN_TAGS, adminTags.toArray()); } - - document.endObject(); - return document; } @@ -334,32 +351,7 @@ public class RuleRegistry { } public PagedResult find(RuleQuery query) { - BoolFilterBuilder mainFilter = boolFilter().mustNot(termFilter(RuleDocument.FIELD_STATUS, STATUS_REMOVED)); - if (StringUtils.isNotBlank(query.searchQuery())) { - mainFilter.must(FilterBuilders.queryFilter( - QueryBuilders.multiMatchQuery(query.searchQuery(), RuleDocument.FIELD_NAME + ".search", RuleDocument.FIELD_KEY).operator(Operator.AND))); - } - addMustTermOrTerms(mainFilter, RuleDocument.FIELD_LANGUAGE, query.languages()); - addMustTermOrTerms(mainFilter, RuleDocument.FIELD_REPOSITORY_KEY, query.repositories()); - addMustTermOrTerms(mainFilter, RuleDocument.FIELD_SEVERITY, query.severities()); - addMustTermOrTerms(mainFilter, RuleDocument.FIELD_STATUS, query.statuses()); - if (!query.tags().isEmpty()) { - mainFilter.must(FilterBuilders.queryFilter( - QueryBuilders.multiMatchQuery(query.tags(), RuleDocument.FIELD_ADMIN_TAGS, RuleDocument.FIELD_SYSTEM_TAGS).operator(Operator.OR)) - ); - } - if (!query.debtCharacteristics().isEmpty()) { - mainFilter.must(FilterBuilders.queryFilter( - QueryBuilders.multiMatchQuery(query.debtCharacteristics(), RuleDocument.FIELD_CHARACTERISTIC_KEY, RuleDocument.FIELD_SUB_CHARACTERISTIC_KEY).operator(Operator.OR)) - ); - } - if (query.hasDebtCharacteristic() != null) { - if (Boolean.TRUE.equals(query.hasDebtCharacteristic())) { - mainFilter.must(FilterBuilders.existsFilter(RuleDocument.FIELD_CHARACTERISTIC_KEY)); - } else { - mainFilter.mustNot(FilterBuilders.existsFilter(RuleDocument.FIELD_CHARACTERISTIC_KEY)); - } - } + BoolFilterBuilder mainFilter = convertRuleQueryToFilterBuilder(query); Builder rulesBuilder = ImmutableList.builder(); SearchRequestBuilder searchRequestBuilder = @@ -401,6 +393,36 @@ public class RuleRegistry { } } + private static BoolFilterBuilder convertRuleQueryToFilterBuilder(RuleQuery query){ + BoolFilterBuilder mainFilter = boolFilter().mustNot(termFilter(RuleDocument.FIELD_STATUS, STATUS_REMOVED)); + if (StringUtils.isNotBlank(query.searchQuery())) { + mainFilter.must(FilterBuilders.queryFilter( + QueryBuilders.multiMatchQuery(query.searchQuery(), RuleDocument.FIELD_NAME + ".search", RuleDocument.FIELD_KEY).operator(Operator.AND))); + } + addMustTermOrTerms(mainFilter, RuleDocument.FIELD_LANGUAGE, query.languages()); + addMustTermOrTerms(mainFilter, RuleDocument.FIELD_REPOSITORY_KEY, query.repositories()); + addMustTermOrTerms(mainFilter, RuleDocument.FIELD_SEVERITY, query.severities()); + addMustTermOrTerms(mainFilter, RuleDocument.FIELD_STATUS, query.statuses()); + if (!query.tags().isEmpty()) { + mainFilter.must(FilterBuilders.queryFilter( + QueryBuilders.multiMatchQuery(query.tags(), RuleDocument.FIELD_ADMIN_TAGS, RuleDocument.FIELD_SYSTEM_TAGS).operator(Operator.OR)) + ); + } + if (!query.debtCharacteristics().isEmpty()) { + mainFilter.must(FilterBuilders.queryFilter( + QueryBuilders.multiMatchQuery(query.debtCharacteristics(), RuleDocument.FIELD_CHARACTERISTIC_KEY, RuleDocument.FIELD_SUB_CHARACTERISTIC_KEY).operator(Operator.OR)) + ); + } + if (query.hasDebtCharacteristic() != null) { + if (Boolean.TRUE.equals(query.hasDebtCharacteristic())) { + mainFilter.must(FilterBuilders.existsFilter(RuleDocument.FIELD_CHARACTERISTIC_KEY)); + } else { + mainFilter.mustNot(FilterBuilders.existsFilter(RuleDocument.FIELD_CHARACTERISTIC_KEY)); + } + } + return mainFilter; + } + private static void addMustTermOrTerms(BoolFilterBuilder filter, String field, Collection terms) { FilterBuilder termOrTerms = getTermOrTerms(field, terms); if (termOrTerms != null) { diff --git a/sonar-server/src/main/java/org/sonar/server/startup/CopyRequirementsFromCharacteristicsToRules.java b/sonar-server/src/main/java/org/sonar/server/startup/CopyRequirementsFromCharacteristicsToRules.java index a7153b0db2a..88aab6ced84 100644 --- a/sonar-server/src/main/java/org/sonar/server/startup/CopyRequirementsFromCharacteristicsToRules.java +++ b/sonar-server/src/main/java/org/sonar/server/startup/CopyRequirementsFromCharacteristicsToRules.java @@ -31,6 +31,7 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.sonar.api.platform.ServerUpgradeStatus; import org.sonar.api.rules.Rule; +import org.sonar.api.server.debt.DebtRemediationFunction; import org.sonar.api.utils.Duration; import org.sonar.api.utils.System2; import org.sonar.core.persistence.Database; @@ -173,50 +174,67 @@ public class CopyRequirementsFromCharacteristicsToRules { } private boolean convert(RuleRow ruleRow, PreparedStatement updateStatement, Collection requirementsForRule) throws SQLException { - RequirementDto enabledRequirement = Iterables.find(requirementsForRule, new Predicate() { - @Override - public boolean apply(RequirementDto input) { - return input.isEnabled(); - } - }, null); + RequirementDto enabledRequirement = enabledRequirement(requirementsForRule); if (enabledRequirement == null && !Rule.STATUS_REMOVED.equals(ruleRow.getStatus())) { // If no enabled requirement is found, it means that the requirement has been disabled for this rule - updateStatement.setInt(1, RuleDto.DISABLED_CHARACTERISTIC_ID); - updateStatement.setNull(2, Types.VARCHAR); - updateStatement.setNull(3, Types.VARCHAR); - updateStatement.setNull(4, Types.VARCHAR); - updateStatement.setTimestamp(5, new Timestamp(system2.now())); - updateStatement.setInt(6, ruleRow.getId()); - return true; + return convertDisableRequirement(ruleRow, updateStatement); } else if (enabledRequirement != null) { // If one requirement is enable, it means either that this requirement has been set from SQALE, or that it come from a XML model definition + return convertEnabledRequirement(ruleRow, updateStatement, enabledRequirement); - ruleRow.setCharacteristicId(enabledRequirement.getParentId()); - ruleRow.setFunction(enabledRequirement.getFunction().toUpperCase()); - ruleRow.setCoefficient(convertDuration(enabledRequirement.getCoefficientValue(), enabledRequirement.getCoefficientUnit())); - ruleRow.setOffset(convertDuration(enabledRequirement.getOffsetValue(), enabledRequirement.getOffsetUnit())); - - // If the coefficient of a linear or linear with offset function is null, it should be replaced by 0 - if (("LINEAR".equals(ruleRow.getFunction()) || "LINEAR_OFFSET".equals(ruleRow.getFunction())) && ruleRow.getCoefficient() == null) { - ruleRow.setCoefficient("0" + convertUnit(enabledRequirement.getCoefficientUnit())); - // If the offset of a constant per issue or linear with offset function is null, it should be replaced by 0 - } else if (("CONSTANT_ISSUE".equals(ruleRow.getFunction()) || "LINEAR_OFFSET".equals(ruleRow.getFunction())) && ruleRow.getOffset() == null) { - ruleRow.setOffset("0" + convertUnit(enabledRequirement.getOffsetUnit())); - } + // When default values on debt are the same that ones set by SQALE, nothing to do + } + return false; + } - if (!isDebtDefaultValuesSameAsOverriddenValues(ruleRow)) { - // Default values on debt are not the same that ones set by SQALE, update the rule - updateStatement.setInt(1, ruleRow.getCharacteristicId()); - updateStatement.setString(2, ruleRow.getFunction()); - updateStatement.setString(3, ruleRow.getCoefficient()); - updateStatement.setString(4, ruleRow.getOffset()); - updateStatement.setTimestamp(5, new Timestamp(system2.now())); - updateStatement.setInt(6, ruleRow.getId()); - return true; + private static RequirementDto enabledRequirement(Collection requirementsForRule) { + return Iterables.find(requirementsForRule, new Predicate() { + @Override + public boolean apply(RequirementDto input) { + return input.isEnabled(); } - // When default values on debt are the same that ones set by SQALE, nothing to do + }, null); + } + + private boolean convertDisableRequirement(RuleRow ruleRow, PreparedStatement updateStatement) throws SQLException { + updateStatement.setInt(1, RuleDto.DISABLED_CHARACTERISTIC_ID); + updateStatement.setNull(2, Types.VARCHAR); + updateStatement.setNull(3, Types.VARCHAR); + updateStatement.setNull(4, Types.VARCHAR); + updateStatement.setTimestamp(5, new Timestamp(system2.now())); + updateStatement.setInt(6, ruleRow.getId()); + return true; + } + + private boolean convertEnabledRequirement(RuleRow ruleRow, PreparedStatement updateStatement, RequirementDto enabledRequirement) throws SQLException { + ruleRow.setCharacteristicId(enabledRequirement.getParentId()); + ruleRow.setFunction(enabledRequirement.getFunction().toUpperCase()); + ruleRow.setCoefficient(convertDuration(enabledRequirement.getCoefficientValue(), enabledRequirement.getCoefficientUnit())); + ruleRow.setOffset(convertDuration(enabledRequirement.getOffsetValue(), enabledRequirement.getOffsetUnit())); + + // If the coefficient of a linear or linear with offset function is null, it should be replaced by 0 + if ((DebtRemediationFunction.Type.LINEAR.name().equals(ruleRow.getFunction()) || + DebtRemediationFunction.Type.LINEAR_OFFSET.name().equals(ruleRow.getFunction())) + && ruleRow.getCoefficient() == null) { + ruleRow.setCoefficient("0" + convertUnit(enabledRequirement.getCoefficientUnit())); + // If the offset of a constant per issue or linear with offset function is null, it should be replaced by 0 + } else if ((DebtRemediationFunction.Type.CONSTANT_ISSUE.name().equals(ruleRow.getFunction()) + || DebtRemediationFunction.Type.LINEAR_OFFSET.name().equals(ruleRow.getFunction())) + && ruleRow.getOffset() == null) { + ruleRow.setOffset("0" + convertUnit(enabledRequirement.getOffsetUnit())); + } + + if (!isDebtDefaultValuesSameAsOverriddenValues(ruleRow)) { + // Default values on debt are not the same that ones set by SQALE, update the rule + updateStatement.setInt(1, ruleRow.getCharacteristicId()); + updateStatement.setString(2, ruleRow.getFunction()); + updateStatement.setString(3, ruleRow.getCoefficient()); + updateStatement.setString(4, ruleRow.getOffset()); + updateStatement.setTimestamp(5, new Timestamp(system2.now())); + updateStatement.setInt(6, ruleRow.getId()); + return true; } return false; } diff --git a/sonar-server/src/test/java/org/sonar/server/rule/RuleOperationsTest.java b/sonar-server/src/test/java/org/sonar/server/rule/RuleOperationsTest.java index 0c6fb636888..39cf65daaf2 100644 --- a/sonar-server/src/test/java/org/sonar/server/rule/RuleOperationsTest.java +++ b/sonar-server/src/test/java/org/sonar/server/rule/RuleOperationsTest.java @@ -533,7 +533,7 @@ public class RuleOperationsTest { } @Test - public void disable_characteristic_and_remove_remediation_function_when_update_rule_with_no_sub_characteristic() throws Exception { + public void disable_rule_debt_when_update_rule_with_no_sub_characteristic() throws Exception { RuleDto dto = new RuleDto().setId(1).setRepositoryKey("squid").setRuleKey("UselessImportCheck") .setDefaultSubCharacteristicId(6).setDefaultRemediationFunction("LINEAR").setDefaultRemediationCoefficient("10min") .setSubCharacteristicId(6).setRemediationFunction("CONSTANT_ISSUE").setRemediationOffset("10min"); @@ -557,6 +557,36 @@ public class RuleOperationsTest { assertThat(result.getUpdatedAt()).isEqualTo(now); } + @Test + public void disable_rule_debt_when_update_rule_with_no_function() throws Exception { + RuleDto dto = new RuleDto().setId(1).setRepositoryKey("squid").setRuleKey("UselessImportCheck") + .setDefaultSubCharacteristicId(6).setDefaultRemediationFunction("CONSTANT_ISSUE").setDefaultRemediationOffset("10min"); + RuleKey ruleKey = RuleKey.of("squid", "UselessImportCheck"); + + when(ruleDao.selectByKey(ruleKey, session)).thenReturn(dto); + + CharacteristicDto subCharacteristic = new CharacteristicDto().setId(2).setKey("COMPILER").setName("Compiler").setParentId(1); + when(characteristicDao.selectByKey("COMPILER", session)).thenReturn(subCharacteristic); + when(characteristicDao.selectById(2, session)).thenReturn(subCharacteristic); + CharacteristicDto characteristic = new CharacteristicDto().setId(1).setKey("PORTABILITY").setName("Portability").setOrder(2); + when(characteristicDao.selectById(1, session)).thenReturn(characteristic); + + operations.updateRule(new RuleChange().setRuleKey(ruleKey).setDebtCharacteristicKey("COMPILER"), authorizedUserSession); + + verify(ruleDao).update(ruleCaptor.capture(), eq(session)); + verify(ruleRegistry).reindex(eq(ruleCaptor.getValue()), eq(session)); + verify(session).commit(); + + RuleDto result = ruleCaptor.getValue(); + + assertThat(result.getId()).isEqualTo(1); + assertThat(result.getSubCharacteristicId()).isEqualTo(-1); + assertThat(result.getRemediationFunction()).isNull(); + assertThat(result.getRemediationCoefficient()).isNull(); + assertThat(result.getRemediationOffset()).isNull(); + assertThat(result.getUpdatedAt()).isEqualTo(now); + } + @Test public void disable_characteristic_on_rule_having_no_debt_info() throws Exception { RuleDto dto = new RuleDto().setId(1).setRepositoryKey("squid").setRuleKey("UselessImportCheck");