From ed8b193aabca70842f9210e46464a26144dc9da5 Mon Sep 17 00:00:00 2001 From: Simon Brandhof Date: Tue, 1 Jul 2014 11:01:12 +0200 Subject: [PATCH] Fix some quality flaws --- .../qualityprofile/ActiveRuleChange.java | 31 +++++++++---------- .../qualityprofile/QProfileService.java | 7 +++-- .../qualityprofile/RuleActivatorContext.java | 6 ++-- .../RuleActivatorContextFactory.java | 5 +-- .../qualityprofile/index/ActiveRuleDoc.java | 2 +- .../org/sonar/server/rule/RuleService.java | 11 ++++++- .../sonar/server/rule/index/RuleQuery.java | 16 +++++----- .../server/rule/ws/ActiveRuleCompleter.java | 10 +++--- .../sonar/server/rule/ws/CreateAction.java | 4 +-- .../sonar/server/rule/ws/UpdateAction.java | 2 +- .../server/rule/RuleServiceMediumTest.java | 25 +++++++++++++++ 11 files changed, 79 insertions(+), 40 deletions(-) diff --git a/sonar-server/src/main/java/org/sonar/server/qualityprofile/ActiveRuleChange.java b/sonar-server/src/main/java/org/sonar/server/qualityprofile/ActiveRuleChange.java index c4348fca565..44391663eec 100644 --- a/sonar-server/src/main/java/org/sonar/server/qualityprofile/ActiveRuleChange.java +++ b/sonar-server/src/main/java/org/sonar/server/qualityprofile/ActiveRuleChange.java @@ -27,6 +27,7 @@ import org.sonar.core.qualityprofile.db.ActiveRuleKey; import javax.annotation.CheckForNull; import javax.annotation.Nullable; + import java.util.HashMap; import java.util.Map; @@ -60,22 +61,21 @@ public class ActiveRuleChange implements ActivityLog { return severity; } - public ActiveRuleChange setSeverity(@Nullable String severity) { - this.severity = severity; + public ActiveRuleChange setSeverity(@Nullable String s) { + this.severity = s; return this; } - public ActiveRuleChange setInheritance(@Nullable ActiveRule.Inheritance inheritance) { - this.inheritance = inheritance; + public ActiveRuleChange setInheritance(@Nullable ActiveRule.Inheritance i) { + this.inheritance = i; return this; } @CheckForNull public ActiveRule.Inheritance getInheritance() { - return this.inheritance; + return inheritance; } - @CheckForNull public Map getParameters() { return parameters; } @@ -95,16 +95,13 @@ public class ActiveRuleChange implements ActivityLog { public Map getDetails() { HashMap details = new HashMap(); - if (getKey() != null) { - details.put("key", getKey().toString()); - details.put("ruleKey", getKey().ruleKey().toString()); - details.put("profileKey", getKey().qProfile().toString()); - } - if (!parameters.isEmpty()) { - for (Map.Entry param : parameters.entrySet()) { - if (!param.getKey().isEmpty()) { - details.put("param_" + param.getKey(), param.getValue()); - } + details.put("key", getKey().toString()); + details.put("ruleKey", getKey().ruleKey().toString()); + details.put("profileKey", getKey().qProfile().toString()); + + for (Map.Entry param : parameters.entrySet()) { + if (!param.getKey().isEmpty()) { + details.put("param_" + param.getKey(), param.getValue()); } } if (StringUtils.isNotEmpty(severity)) { @@ -133,6 +130,6 @@ public class ActiveRuleChange implements ActivityLog { @Override public String getAction() { - return this.type.name(); + return type.name(); } } diff --git a/sonar-server/src/main/java/org/sonar/server/qualityprofile/QProfileService.java b/sonar-server/src/main/java/org/sonar/server/qualityprofile/QProfileService.java index 45d196b3f1b..7f496b39ea3 100644 --- a/sonar-server/src/main/java/org/sonar/server/qualityprofile/QProfileService.java +++ b/sonar-server/src/main/java/org/sonar/server/qualityprofile/QProfileService.java @@ -218,8 +218,11 @@ public class QProfileService implements ServerComponent { RuleDto ruleDto = db.ruleDao().getNullableByKey(session, profileActivity.ruleKey()); profileActivity.ruleName(ruleDto != null ? ruleDto.getName() : null); - UserDto user = db.userDao().selectActiveUserByLogin(profileActivity.login(), session); - profileActivity.authorName(user != null ? user.getName() : null); + String login = profileActivity.login(); + if (login != null) { + UserDto user = db.userDao().selectActiveUserByLogin(login, session); + profileActivity.authorName(user != null ? user.getName() : null); + } result.getHits().add(profileActivity); } return result; diff --git a/sonar-server/src/main/java/org/sonar/server/qualityprofile/RuleActivatorContext.java b/sonar-server/src/main/java/org/sonar/server/qualityprofile/RuleActivatorContext.java index 1400427ab42..81c470690ba 100644 --- a/sonar-server/src/main/java/org/sonar/server/qualityprofile/RuleActivatorContext.java +++ b/sonar-server/src/main/java/org/sonar/server/qualityprofile/RuleActivatorContext.java @@ -204,10 +204,12 @@ class RuleActivatorContext { } boolean isSame(ActiveRuleChange change) { - if (change.getInheritance() != null && !change.getInheritance().name().equals(activeRule.getInheritance())) { + ActiveRule.Inheritance inheritance = change.getInheritance(); + if (inheritance != null && !inheritance.name().equals(activeRule.getInheritance())) { return false; } - if (change.getSeverity() != null && !change.getSeverity().equals(activeRule.getSeverityString())) { + String severity = change.getSeverity(); + if (severity != null && !severity.equals(activeRule.getSeverityString())) { return false; } for (Map.Entry changeParam : change.getParameters().entrySet()) { diff --git a/sonar-server/src/main/java/org/sonar/server/qualityprofile/RuleActivatorContextFactory.java b/sonar-server/src/main/java/org/sonar/server/qualityprofile/RuleActivatorContextFactory.java index bf8fec54f75..1854942476b 100644 --- a/sonar-server/src/main/java/org/sonar/server/qualityprofile/RuleActivatorContextFactory.java +++ b/sonar-server/src/main/java/org/sonar/server/qualityprofile/RuleActivatorContextFactory.java @@ -63,8 +63,9 @@ public class RuleActivatorContextFactory implements ServerComponent { private RuleActivatorContext create(QualityProfileDto profile, RuleKey ruleKey, DbSession session, RuleActivatorContext context) { initRule(ruleKey, context, session); initActiveRules(profile.getKey(), ruleKey, context, session, false); - if (profile.getParentKee() != null) { - initActiveRules(profile.getParentKee(), ruleKey, context, session, true); + String parentKee = profile.getParentKee(); + if (parentKee != null) { + initActiveRules(parentKee, ruleKey, context, session, true); } return context; } 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 ca07838e2da..24ce64dd9e4 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) getNullableField(ActiveRuleNormalizer.ActiveRuleField.KEY.field())); + this.key = ActiveRuleKey.parse((String) getField(ActiveRuleNormalizer.ActiveRuleField.KEY.field())); Preconditions.checkArgument(key!=null, "Invalid ActiveRuleKey!"); } diff --git a/sonar-server/src/main/java/org/sonar/server/rule/RuleService.java b/sonar-server/src/main/java/org/sonar/server/rule/RuleService.java index 133c066d56f..924490bcb4a 100644 --- a/sonar-server/src/main/java/org/sonar/server/rule/RuleService.java +++ b/sonar-server/src/main/java/org/sonar/server/rule/RuleService.java @@ -22,6 +22,7 @@ package org.sonar.server.rule; import org.sonar.api.ServerComponent; import org.sonar.api.rule.RuleKey; import org.sonar.core.permission.GlobalPermissions; +import org.sonar.server.exceptions.NotFoundException; import org.sonar.server.rule.index.RuleIndex; import org.sonar.server.rule.index.RuleNormalizer; import org.sonar.server.rule.index.RuleQuery; @@ -51,10 +52,18 @@ public class RuleService implements ServerComponent { } @CheckForNull - public org.sonar.server.rule.Rule getByKey(RuleKey key) { + public Rule getByKey(RuleKey key) { return index.getByKey(key); } + public Rule getNonNullByKey(RuleKey key) { + Rule rule = index.getByKey(key); + if (rule == null) { + throw new NotFoundException("Rule not found: " + key); + } + return rule; + } + public RuleQuery newRuleQuery() { return new RuleQuery(); } diff --git a/sonar-server/src/main/java/org/sonar/server/rule/index/RuleQuery.java b/sonar-server/src/main/java/org/sonar/server/rule/index/RuleQuery.java index 898acb5d657..57f186a1d89 100644 --- a/sonar-server/src/main/java/org/sonar/server/rule/index/RuleQuery.java +++ b/sonar-server/src/main/java/org/sonar/server/rule/index/RuleQuery.java @@ -65,8 +65,8 @@ public class RuleQuery { return qProfileKey; } - public RuleQuery setQProfileKey(String qProfileKey) { - this.qProfileKey = qProfileKey; + public RuleQuery setQProfileKey(@Nullable String s) { + this.qProfileKey = s; return this; } @@ -229,8 +229,8 @@ public class RuleQuery { return this; } - public RuleQuery setAvailableSince(Date since) { - this.availableSince = since; + public RuleQuery setAvailableSince(@Nullable Date d) { + this.availableSince = d; return this; } @@ -238,8 +238,8 @@ public class RuleQuery { return this.availableSince; } - public RuleQuery setInternalKey(String internalKey) { - this.internalKey = internalKey; + public RuleQuery setInternalKey(@Nullable String s) { + this.internalKey = s; return this; } @@ -247,8 +247,8 @@ public class RuleQuery { return internalKey; } - public RuleQuery setRuleKey(String ruleKey) { - this.ruleKey = ruleKey; + public RuleQuery setRuleKey(@Nullable String s) { + this.ruleKey = s; return this; } diff --git a/sonar-server/src/main/java/org/sonar/server/rule/ws/ActiveRuleCompleter.java b/sonar-server/src/main/java/org/sonar/server/rule/ws/ActiveRuleCompleter.java index 5f90f91c963..bb8f2da4ee3 100644 --- a/sonar-server/src/main/java/org/sonar/server/rule/ws/ActiveRuleCompleter.java +++ b/sonar-server/src/main/java/org/sonar/server/rule/ws/ActiveRuleCompleter.java @@ -46,10 +46,11 @@ public class ActiveRuleCompleter implements ServerComponent { void completeSearch(RuleQuery query, Collection rules, JsonWriter json) { json.name("actives").beginObject(); - if (query.getQProfileKey() != null) { + String profileKey = query.getQProfileKey(); + if (profileKey != null) { // Load details of active rules on the selected profile for (Rule rule : rules) { - ActiveRule activeRule = loader.getActiveRule(ActiveRuleKey.of(query.getQProfileKey(), rule.key())); + ActiveRule activeRule = loader.getActiveRule(ActiveRuleKey.of(profileKey, rule.key())); if (activeRule != null) { writeActiveRules(rule.key(), Arrays.asList(activeRule), json); } @@ -88,8 +89,9 @@ public class ActiveRuleCompleter implements ServerComponent { .prop("qProfile", activeRule.key().qProfile().toString()) .prop("inherit", activeRule.inheritance().toString()) .prop("severity", activeRule.severity()); - if (activeRule.parentKey() != null) { - json.prop("parent", activeRule.parentKey().toString()); + ActiveRuleKey parentKey = activeRule.parentKey(); + if (parentKey != null) { + json.prop("parent", parentKey.toString()); } json.name("params").beginArray(); for (Map.Entry param : activeRule.params().entrySet()) { diff --git a/sonar-server/src/main/java/org/sonar/server/rule/ws/CreateAction.java b/sonar-server/src/main/java/org/sonar/server/rule/ws/CreateAction.java index 3eb19d41e9d..58a2cf69cf0 100644 --- a/sonar-server/src/main/java/org/sonar/server/rule/ws/CreateAction.java +++ b/sonar-server/src/main/java/org/sonar/server/rule/ws/CreateAction.java @@ -156,14 +156,14 @@ public class CreateAction implements RequestHandler { } private void writeResponse(Response response, RuleKey ruleKey) { - Rule rule = service.getByKey(ruleKey); + Rule rule = service.getNonNullByKey(ruleKey); JsonWriter json = response.newJsonWriter().beginObject().name("rule"); mapping.write((BaseDoc) rule, json); json.endObject().close(); } private void write409(Response response, RuleKey ruleKey) { - Rule rule = service.getByKey(ruleKey); + Rule rule = service.getNonNullByKey(ruleKey); Response.Stream stream = response.stream(); stream.setStatus(409); diff --git a/sonar-server/src/main/java/org/sonar/server/rule/ws/UpdateAction.java b/sonar-server/src/main/java/org/sonar/server/rule/ws/UpdateAction.java index cca752f610b..553491f93dc 100644 --- a/sonar-server/src/main/java/org/sonar/server/rule/ws/UpdateAction.java +++ b/sonar-server/src/main/java/org/sonar/server/rule/ws/UpdateAction.java @@ -213,7 +213,7 @@ public class UpdateAction implements RequestHandler { } private void writeResponse(Response response, RuleKey ruleKey) { - Rule rule = service.getByKey(ruleKey); + Rule rule = service.getNonNullByKey(ruleKey); JsonWriter json = response.newJsonWriter().beginObject().name("rule"); mapping.write((BaseDoc) rule, json); json.endObject().close(); diff --git a/sonar-server/src/test/java/org/sonar/server/rule/RuleServiceMediumTest.java b/sonar-server/src/test/java/org/sonar/server/rule/RuleServiceMediumTest.java index 0147e02f3e4..9c6f692fae2 100644 --- a/sonar-server/src/test/java/org/sonar/server/rule/RuleServiceMediumTest.java +++ b/sonar-server/src/test/java/org/sonar/server/rule/RuleServiceMediumTest.java @@ -32,6 +32,7 @@ import org.sonar.core.permission.GlobalPermissions; import org.sonar.core.persistence.DbSession; import org.sonar.core.rule.RuleDto; import org.sonar.server.db.DbClient; +import org.sonar.server.exceptions.NotFoundException; import org.sonar.server.exceptions.UnauthorizedException; import org.sonar.server.rule.db.RuleDao; import org.sonar.server.rule.index.RuleIndex; @@ -43,6 +44,7 @@ import java.util.Collections; import java.util.Set; import static org.fest.assertions.Assertions.assertThat; +import static org.fest.assertions.Fail.fail; public class RuleServiceMediumTest { @@ -79,6 +81,29 @@ public class RuleServiceMediumTest { Rule rule = service.getByKey(key); assertThat(rule).isNotNull(); + + assertThat(service.getByKey(RuleKey.of("un", "known"))).isNull(); + } + + @Test + public void get_non_null_rule_by_key() throws Exception { + MockUserSession.set() + .setGlobalPermissions(GlobalPermissions.QUALITY_PROFILE_ADMIN) + .setLogin("me"); + + RuleKey key = RuleKey.of("java", "S001"); + + dao.insert(dbSession, RuleTesting.newDto(key)); + dbSession.commit(); + dbSession.clearCache(); + + assertThat(service.getNonNullByKey(key)).isNotNull(); + try { + service.getNonNullByKey(RuleKey.of("un", "known")); + fail(); + } catch (NotFoundException e) { + assertThat(e).hasMessage("Rule not found: un:known"); + } } @Test -- 2.39.5