From 72a18184d39f558feb872a641b767a1c1e460ef8 Mon Sep 17 00:00:00 2001 From: Simon Brandhof Date: Mon, 26 May 2014 18:21:38 +0200 Subject: [PATCH] SONAR-5007 fix some quality flaws --- .../RegisterQualityProfiles.java | 20 ++--- .../qualityprofile/index/ActiveRuleDoc.java | 10 +-- .../org/sonar/server/rule/RegisterRules.java | 19 +++-- .../org/sonar/server/rule/db/RuleDao.java | 3 + .../org/sonar/server/rule/index/RuleDoc.java | 74 +++++++------------ .../java/org/sonar/server/search/BaseDoc.java | 24 ++++-- .../org/sonar/server/search/IndexUtils.java | 2 +- .../sonar/server/search/ws/BaseMapping.java | 8 +- .../WEB-INF/app/views/issue/_rule.html.erb | 4 +- .../org/sonar/server/search/BaseDocTest.java | 8 +- 10 files changed, 85 insertions(+), 87 deletions(-) diff --git a/sonar-server/src/main/java/org/sonar/server/qualityprofile/RegisterQualityProfiles.java b/sonar-server/src/main/java/org/sonar/server/qualityprofile/RegisterQualityProfiles.java index 4995e17ec6e..ee8eadc2947 100644 --- a/sonar-server/src/main/java/org/sonar/server/qualityprofile/RegisterQualityProfiles.java +++ b/sonar-server/src/main/java/org/sonar/server/qualityprofile/RegisterQualityProfiles.java @@ -93,10 +93,10 @@ public class RegisterQualityProfiles implements ServerComponent { try { ListMultimap profilesByLanguage = profilesByLanguage(); for (String language : profilesByLanguage.keySet()) { - List profiles = profilesByLanguage.get(language); - verifyLanguage(language, profiles); + List profileDefs = profilesByLanguage.get(language); + verifyLanguage(language, profileDefs); - for (Map.Entry> entry : profilesByName(profiles).entrySet()) { + for (Map.Entry> entry : profilesByName(profileDefs).entrySet()) { String profileName = entry.getKey(); QualityProfileKey profileKey = QualityProfileKey.of(profileName, language); if (shouldRegister(profileKey, session)) { @@ -104,7 +104,7 @@ public class RegisterQualityProfiles implements ServerComponent { } defaultProfilesCache.put(language, profileName); } - setDefault(language, profiles, session); + setDefault(language, profileDefs, session); } session.commit(); } finally { @@ -161,21 +161,21 @@ public class RegisterQualityProfiles implements ServerComponent { session.commit(); } - private void setDefault(String language, List profiles, DbSession session) { + private void setDefault(String language, List profileDefs, DbSession session) { String propertyKey = "sonar.profile." + language; boolean upToDate = false; String currentDefault = settings.getString(propertyKey); if (currentDefault != null) { - for (RulesProfile profile : profiles) { - if (StringUtils.equals(currentDefault, profile.getName())) { - upToDate = true; - } + // check validity + QualityProfileDto profile = dbClient.qualityProfileDao().getByKey(QualityProfileKey.of(currentDefault, language), session); + if (profile != null) { + upToDate = true; } } if (!upToDate) { - String defaultProfileName = defaultProfileName(profiles); + String defaultProfileName = defaultProfileName(profileDefs); LOGGER.info("Set default " + language + " profile: " + defaultProfileName); settings.saveProperty(propertyKey, defaultProfileName); } diff --git a/sonar-server/src/main/java/org/sonar/server/qualityprofile/index/ActiveRuleDoc.java b/sonar-server/src/main/java/org/sonar/server/qualityprofile/index/ActiveRuleDoc.java index e6555b786eb..bccc4735f49 100644 --- a/sonar-server/src/main/java/org/sonar/server/qualityprofile/index/ActiveRuleDoc.java +++ b/sonar-server/src/main/java/org/sonar/server/qualityprofile/index/ActiveRuleDoc.java @@ -35,7 +35,7 @@ public class ActiveRuleDoc extends BaseDoc implements ActiveRule { public ActiveRuleDoc(Map fields) { super(fields); - this.key = ActiveRuleKey.parse((String) getField(ActiveRuleNormalizer.ActiveRuleField.KEY.key())); + this.key = ActiveRuleKey.parse((String) getNullableField(ActiveRuleNormalizer.ActiveRuleField.KEY.key())); Preconditions.checkArgument(key!=null, "Invalid ActiveRuleKey!"); } @@ -46,12 +46,12 @@ public class ActiveRuleDoc extends BaseDoc implements ActiveRule { @Override public String severity() { - return (String) getField(ActiveRuleNormalizer.ActiveRuleField.SEVERITY.key()); + return (String) getNullableField(ActiveRuleNormalizer.ActiveRuleField.SEVERITY.key()); } @Override public ActiveRule.Inheritance inheritance() { - String inheritance = getField(ActiveRuleNormalizer.ActiveRuleField.INHERITANCE.key()); + String inheritance = getNullableField(ActiveRuleNormalizer.ActiveRuleField.INHERITANCE.key()); if (inheritance == null || inheritance.isEmpty() || inheritance.toLowerCase().contains("none")) { return Inheritance.NONE; @@ -67,7 +67,7 @@ public class ActiveRuleDoc extends BaseDoc implements ActiveRule { @Override @CheckForNull public ActiveRuleKey parentKey() { - String data = getField(ActiveRuleNormalizer.ActiveRuleField.PARENT_KEY.key()); + String data = getNullableField(ActiveRuleNormalizer.ActiveRuleField.PARENT_KEY.key()); if (data != null && !data.isEmpty()) { return ActiveRuleKey.parse(data); } @@ -77,7 +77,7 @@ public class ActiveRuleDoc extends BaseDoc implements ActiveRule { @Override public Map params() { Map params = new HashMap(); - List> allParams = getField(ActiveRuleNormalizer.ActiveRuleField.PARAMS.key()); + List> allParams = getNullableField(ActiveRuleNormalizer.ActiveRuleField.PARAMS.key()); if (allParams != null) { for (Map param : allParams) { params.put(param.get(ActiveRuleNormalizer.ActiveRuleParamField.NAME.key()), 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 f8e1f351935..ed72f4c20e7 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 @@ -230,12 +230,15 @@ public class RegisterRules implements Startable { // 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); + if (hasDebt) { + return mergeDebtDefinitions(def, dto, + subCharacteristic.getId(), + debtRemediationFunction.type().name(), + debtRemediationFunction.coefficient(), + debtRemediationFunction.offset(), + def.effortToFixDescription()); + } + return mergeDebtDefinitions(def, dto, null, null, null, null, null); } private boolean mergeDebtDefinitions(RulesDefinition.Rule def, RuleDto dto, @Nullable Integer characteristicId, @Nullable String remediationFunction, @@ -354,8 +357,8 @@ public class RegisterRules implements Startable { if (toBeRemoved && !RuleStatus.REMOVED.toString().equals(ruleDto.getStatus())) { LOG.info(String.format("Disable rule %s", ruleDto.getKey())); ruleDto.setStatus(Rule.STATUS_REMOVED); - ruleDto.setSystemTags(Collections.EMPTY_SET); - ruleDto.setTags(Collections.EMPTY_SET); + ruleDto.setSystemTags(Collections.emptySet()); + ruleDto.setTags(Collections.emptySet()); dbClient.ruleDao().update(session, ruleDto); removedRules.add(ruleDto); if (removedRules.size() % 100 == 0) { diff --git a/sonar-server/src/main/java/org/sonar/server/rule/db/RuleDao.java b/sonar-server/src/main/java/org/sonar/server/rule/db/RuleDao.java index 285922d9c41..12605c979c6 100644 --- a/sonar-server/src/main/java/org/sonar/server/rule/db/RuleDao.java +++ b/sonar-server/src/main/java/org/sonar/server/rule/db/RuleDao.java @@ -78,6 +78,9 @@ public class RuleDao extends BaseDao { throw new UnsupportedOperationException("Rules cannot be deleted"); } + /** + * @deprecated in 4.4. Use keys. + */ @CheckForNull @Deprecated public RuleDto getById(int id, DbSession session) { diff --git a/sonar-server/src/main/java/org/sonar/server/rule/index/RuleDoc.java b/sonar-server/src/main/java/org/sonar/server/rule/index/RuleDoc.java index 3a4f0c721cb..0367203ad16 100644 --- a/sonar-server/src/main/java/org/sonar/server/rule/index/RuleDoc.java +++ b/sonar-server/src/main/java/org/sonar/server/rule/index/RuleDoc.java @@ -29,7 +29,6 @@ import org.sonar.server.rule.RuleParam; import org.sonar.server.search.BaseDoc; import org.sonar.server.search.IndexUtils; -import javax.annotation.CheckForNull; import java.util.ArrayList; import java.util.Date; import java.util.List; @@ -46,12 +45,7 @@ public class RuleDoc extends BaseDoc implements Rule { @Override public RuleKey key() { - String key = getField(RuleNormalizer.RuleField.KEY.key()); - if (key == null || key.isEmpty()) { - throw new IllegalStateException("Missing values for RuleKey in RuleDoc"); - } else { - return RuleKey.parse(key); - } + return RuleKey.parse(this.getField(RuleNormalizer.RuleField.KEY.key())); } /** @@ -62,97 +56,82 @@ public class RuleDoc extends BaseDoc implements Rule { } @Override - @CheckForNull public String internalKey() { - return getField(RuleNormalizer.RuleField.INTERNAL_KEY.key()); - } - - @Override - public String markdownNote() { - return getField(RuleNormalizer.RuleField.NOTE.key()); + return getNullableField(RuleNormalizer.RuleField.INTERNAL_KEY.key()); } @Override - @CheckForNull public String language() { return getField(RuleNormalizer.RuleField.LANGUAGE.key()); } @Override - @CheckForNull public String name() { return getField(RuleNormalizer.RuleField.NAME.key()); } @Override - @CheckForNull public String htmlDescription() { - return getField(RuleNormalizer.RuleField.HTML_DESCRIPTION.key()); + return getNullableField(RuleNormalizer.RuleField.HTML_DESCRIPTION.key()); } @Override - @CheckForNull public String severity() { return (String) getField(RuleNormalizer.RuleField.SEVERITY.key()); } @Override - @CheckForNull public RuleStatus status() { return RuleStatus.valueOf((String) getField(RuleNormalizer.RuleField.STATUS.key())); } @Override - @CheckForNull public boolean template() { return (Boolean) getField(RuleNormalizer.RuleField.TEMPLATE.key()); } @Override - @CheckForNull public List tags() { return (List) getField(RuleNormalizer.RuleField.TAGS.key()); } @Override - @CheckForNull public List systemTags() { return (List) getField(RuleNormalizer.RuleField.SYSTEM_TAGS.key()); } @Override - @CheckForNull public List params() { List params = new ArrayList(); - if (this.getField(RuleNormalizer.RuleField.PARAMS.key()) != null) { - List> esParams = this.getField(RuleNormalizer.RuleField.PARAMS.key()); - for (final Map param : esParams) { + List> esParams = getNullableField(RuleNormalizer.RuleField.PARAMS.key()); + if (esParams != null) { + for (final Map esParam : esParams) { params.add(new RuleParam() { { - this.fields = param; + this.fields = esParam; } Map fields; @Override public String key() { - return (String) param.get(RuleNormalizer.RuleParamField.NAME.key()); + return (String) esParam.get(RuleNormalizer.RuleParamField.NAME.key()); } @Override public String description() { - return (String) param.get(RuleNormalizer.RuleParamField.DESCRIPTION.key()); + return (String) esParam.get(RuleNormalizer.RuleParamField.DESCRIPTION.key()); } @Override public String defaultValue() { - return (String) param.get(RuleNormalizer.RuleParamField.DEFAULT_VALUE.key()); + return (String) esParam.get(RuleNormalizer.RuleParamField.DEFAULT_VALUE.key()); } @Override public RuleParamType type() { return RuleParamType - .parse((String) param.get(RuleNormalizer.RuleParamField.TYPE.key())); + .parse((String) esParam.get(RuleNormalizer.RuleParamField.TYPE.key())); } }); } @@ -161,21 +140,18 @@ public class RuleDoc extends BaseDoc implements Rule { } @Override - @CheckForNull public String debtCharacteristicKey() { - return (String) getField(RuleNormalizer.RuleField.CHARACTERISTIC.key()); + return (String) getNullableField(RuleNormalizer.RuleField.CHARACTERISTIC.key()); } @Override - @CheckForNull public String debtSubCharacteristicKey() { - return (String) getField(RuleNormalizer.RuleField.SUB_CHARACTERISTIC.key()); + return (String) getNullableField(RuleNormalizer.RuleField.SUB_CHARACTERISTIC.key()); } @Override - @CheckForNull public DebtRemediationFunction debtRemediationFunction() { - final String function = this.getField(RuleNormalizer.RuleField.DEBT_FUNCTION_TYPE.key()); + final String function = getNullableField(RuleNormalizer.RuleField.DEBT_FUNCTION_TYPE.key()); if (function == null || function.isEmpty()) { return null; } else { @@ -187,43 +163,45 @@ public class RuleDoc extends BaseDoc implements Rule { @Override public String coefficient() { - return (String) getField(RuleNormalizer.RuleField.DEBT_FUNCTION_COEFFICIENT.key()); + return (String) getNullableField(RuleNormalizer.RuleField.DEBT_FUNCTION_COEFFICIENT.key()); } @Override public String offset() { - return (String) getField(RuleNormalizer.RuleField.DEBT_FUNCTION_OFFSET.key()); + return (String) getNullableField(RuleNormalizer.RuleField.DEBT_FUNCTION_OFFSET.key()); } }; } } @Override - @CheckForNull + public String markdownNote() { + return getNullableField(RuleNormalizer.RuleField.NOTE.key()); + } + + @Override public String noteLogin() { - return (String) getField(RuleNormalizer.RuleField.NOTE_LOGIN.key()); + return (String) getNullableField(RuleNormalizer.RuleField.NOTE_LOGIN.key()); } @Override public Date noteCreatedAt() { - return IndexUtils.parseDateTime((String) getField(RuleNormalizer.RuleField.NOTE_CREATED_AT.key())); + return IndexUtils.parseDateTime((String) getNullableField(RuleNormalizer.RuleField.NOTE_CREATED_AT.key())); } @Override public Date noteUpdatedAt() { - return IndexUtils.parseDateTime((String) getField(RuleNormalizer.RuleField.NOTE_UPDATED_AT.key())); + return IndexUtils.parseDateTime((String) getNullableField(RuleNormalizer.RuleField.NOTE_UPDATED_AT.key())); } @Override - @CheckForNull public Date createdAt() { - return IndexUtils.parseDateTime((String) getField(RuleNormalizer.RuleField.CREATED_AT.key())); + return IndexUtils.parseDateTime((String) getNullableField(RuleNormalizer.RuleField.CREATED_AT.key())); } @Override - @CheckForNull public Date updatedAt() { - return IndexUtils.parseDateTime((String) getField(RuleNormalizer.RuleField.UPDATED_AT.key())); + return IndexUtils.parseDateTime((String) getNullableField(RuleNormalizer.RuleField.UPDATED_AT.key())); } @Override diff --git a/sonar-server/src/main/java/org/sonar/server/search/BaseDoc.java b/sonar-server/src/main/java/org/sonar/server/search/BaseDoc.java index 966e714ea44..d7b930532fc 100644 --- a/sonar-server/src/main/java/org/sonar/server/search/BaseDoc.java +++ b/sonar-server/src/main/java/org/sonar/server/search/BaseDoc.java @@ -27,21 +27,35 @@ import java.util.Map; */ public abstract class BaseDoc { - private final Map fields; + private final Map fields; - protected BaseDoc(Map fields) { + protected BaseDoc(Map fields) { this.fields = fields; } public String keyField() { - return (String)fields.get("key"); + return (String) fields.get("key"); } + /** + * Use this method when field value can be null + */ @CheckForNull - public K getField(String key) { + public K getNullableField(String key) { if (!fields.containsKey(key)) { throw new IllegalStateException(String.format("Field %s not specified in query options", key)); } - return (K)fields.get(key); + return (K) fields.get(key); + } + + /** + * Use this method when you are sure that the value can't be null in ES document + */ + public K getField(String key) { + K value = getNullableField(key); + if (value == null) { + throw new IllegalStateException("Value of index field is null: " + key); + } + return value; } } diff --git a/sonar-server/src/main/java/org/sonar/server/search/IndexUtils.java b/sonar-server/src/main/java/org/sonar/server/search/IndexUtils.java index febc8dcfcff..02d69bb96ce 100644 --- a/sonar-server/src/main/java/org/sonar/server/search/IndexUtils.java +++ b/sonar-server/src/main/java/org/sonar/server/search/IndexUtils.java @@ -34,11 +34,11 @@ public class IndexUtils { @CheckForNull public static Date parseDateTime(@Nullable String s) { - DateFormat sdf = new SimpleDateFormat("yyyy-MM-dd'T'HH:mm:ss.SSS'Z'"); if (s == null) { return null; } try { + DateFormat sdf = new SimpleDateFormat("yyyy-MM-dd'T'HH:mm:ss.SSS'Z'"); return sdf.parse(s); } catch (ParseException e) { throw new IllegalArgumentException("Cannot parse ES date: " + s, e); diff --git a/sonar-server/src/main/java/org/sonar/server/search/ws/BaseMapping.java b/sonar-server/src/main/java/org/sonar/server/search/ws/BaseMapping.java index 28aedaa8ff2..584df5f31a0 100644 --- a/sonar-server/src/main/java/org/sonar/server/search/ws/BaseMapping.java +++ b/sonar-server/src/main/java/org/sonar/server/search/ws/BaseMapping.java @@ -132,7 +132,7 @@ public abstract class BaseMapping implements ServerComponent { @Override public void write(JsonWriter json, BaseDoc doc) { - Object val = doc.getField(indexFields[0]); + Object val = doc.getNullableField(indexFields[0]); json.prop(key, val != null ? val.toString() : null); } } @@ -147,7 +147,7 @@ public abstract class BaseMapping implements ServerComponent { @Override public void write(JsonWriter json, BaseDoc doc) { - Boolean val = doc.getField(indexFields[0]); + Boolean val = doc.getNullableField(indexFields[0]); json.prop(key, val != null ? val.booleanValue() : null); } } @@ -162,7 +162,7 @@ public abstract class BaseMapping implements ServerComponent { @Override public void write(JsonWriter json, BaseDoc doc) { - Iterable values = doc.getField(indexFields[0]); + Iterable values = doc.getNullableField(indexFields[0]); json.name(key).beginArray().values(values).endArray(); } } @@ -177,7 +177,7 @@ public abstract class BaseMapping implements ServerComponent { @Override public void write(JsonWriter json, BaseDoc doc) { - String val = doc.getField(indexFields[0]); + String val = doc.getNullableField(indexFields[0]); if (val != null) { json.propDateTime(key, IndexUtils.parseDateTime(val)); } diff --git a/sonar-server/src/main/webapp/WEB-INF/app/views/issue/_rule.html.erb b/sonar-server/src/main/webapp/WEB-INF/app/views/issue/_rule.html.erb index 360b2c7afd4..588ecab7ef5 100644 --- a/sonar-server/src/main/webapp/WEB-INF/app/views/issue/_rule.html.erb +++ b/sonar-server/src/main/webapp/WEB-INF/app/views/issue/_rule.html.erb @@ -8,9 +8,9 @@ <% end %> -<% if @rule.noteAsMarkdown() %> +<% if @rule.markdownNote() %>
- <%= Api::Utils.markdown_to_html(@rule.noteAsMarkdown()) -%> + <%= Api::Utils.markdown_to_html(@rule.markdownNote()) -%>
<% end %> diff --git a/sonar-server/src/test/java/org/sonar/server/search/BaseDocTest.java b/sonar-server/src/test/java/org/sonar/server/search/BaseDocTest.java index 41ad2775902..2b22562bddf 100644 --- a/sonar-server/src/test/java/org/sonar/server/search/BaseDocTest.java +++ b/sonar-server/src/test/java/org/sonar/server/search/BaseDocTest.java @@ -39,9 +39,9 @@ public class BaseDocTest { BaseDoc doc = new BaseDoc(fields) { }; - assertThat(doc.getField("a_string")).isEqualTo("foo"); - assertThat(doc.getField("a_int")).isEqualTo(42); - assertThat(doc.getField("a_null")).isNull(); + assertThat(doc.getNullableField("a_string")).isEqualTo("foo"); + assertThat(doc.getNullableField("a_int")).isEqualTo(42); + assertThat(doc.getNullableField("a_null")).isNull(); } @Test @@ -51,7 +51,7 @@ public class BaseDocTest { }; try { - doc.getField("a_string"); + doc.getNullableField("a_string"); fail(); } catch (IllegalStateException e) { assertThat(e).hasMessage("Field a_string not specified in query options"); -- 2.39.5