From cbeacdfd2deca921a2f8c5b6663d2c701963faa7 Mon Sep 17 00:00:00 2001 From: Julien Lancelot Date: Thu, 11 Jul 2013 12:08:54 +0200 Subject: [PATCH] Fix quality flaws --- .../sonar/api/config/PropertyDefinition.java | 44 +++++++++----- .../java/org/sonar/api/issue/IssueQuery.java | 4 +- .../issue/InternalRubyIssueService.java | 24 ++++---- .../server/exceptions/HttpExceptionTest.java | 57 +++++++++++++++++++ .../wsclient/services/ResourceQueryTest.java | 48 ++++++++++------ .../services/TimeMachineQueryTest.java | 36 ++++++++---- .../wsclient/services/ViolationQueryTest.java | 44 +++++++++----- .../AuthenticationUnmarshallerTest.java | 18 ++++++ 8 files changed, 204 insertions(+), 71 deletions(-) create mode 100644 sonar-server/src/test/java/org/sonar/server/exceptions/HttpExceptionTest.java diff --git a/sonar-plugin-api/src/main/java/org/sonar/api/config/PropertyDefinition.java b/sonar-plugin-api/src/main/java/org/sonar/api/config/PropertyDefinition.java index d1a7836d689..038c7182c20 100644 --- a/sonar-plugin-api/src/main/java/org/sonar/api/config/PropertyDefinition.java +++ b/sonar-plugin-api/src/main/java/org/sonar/api/config/PropertyDefinition.java @@ -32,6 +32,7 @@ import org.sonar.api.ServerExtension; import org.sonar.api.resources.Qualifiers; import javax.annotation.Nullable; + import java.util.Arrays; import java.util.List; @@ -144,28 +145,41 @@ public final class PropertyDefinition implements BatchExtension, ServerExtension public static Result validate(PropertyType type, @Nullable String value, List options) { if (StringUtils.isNotBlank(value)) { if (type == PropertyType.BOOLEAN) { - if (!StringUtils.equalsIgnoreCase(value, "true") && !StringUtils.equalsIgnoreCase(value, "false")) { - return Result.newError("notBoolean"); - } + return validateBoolean(value); } else if (type == PropertyType.INTEGER) { - if (!NumberUtils.isDigits(value)) { - return Result.newError("notInteger"); - } + return validateInteger(value); } else if (type == PropertyType.FLOAT) { - try { - Double.parseDouble(value); - } catch (NumberFormatException e) { - return Result.newError("notFloat"); - } - } else if (type == PropertyType.SINGLE_SELECT_LIST) { - if (!options.contains(value)) { - return Result.newError("notInOptions"); - } + return validateFloat(value); + } else if (type == PropertyType.SINGLE_SELECT_LIST && !options.contains(value)) { + return Result.newError("notInOptions"); } } return Result.SUCCESS; } + private static Result validateBoolean(@Nullable String value) { + if (!StringUtils.equalsIgnoreCase(value, "true") && !StringUtils.equalsIgnoreCase(value, "false")) { + return Result.newError("notBoolean"); + } + return Result.SUCCESS; + } + + private static Result validateInteger(@Nullable String value) { + if (!NumberUtils.isDigits(value)) { + return Result.newError("notInteger"); + } + return Result.SUCCESS; + } + + private static Result validateFloat(@Nullable String value) { + try { + Double.parseDouble(value); + return Result.SUCCESS; + } catch (NumberFormatException e) { + return Result.newError("notFloat"); + } + } + public Result validate(@Nullable String value) { return validate(type, value, options); } diff --git a/sonar-plugin-api/src/main/java/org/sonar/api/issue/IssueQuery.java b/sonar-plugin-api/src/main/java/org/sonar/api/issue/IssueQuery.java index 98ebbe94073..23e21d30ab6 100644 --- a/sonar-plugin-api/src/main/java/org/sonar/api/issue/IssueQuery.java +++ b/sonar-plugin-api/src/main/java/org/sonar/api/issue/IssueQuery.java @@ -162,12 +162,12 @@ public class IssueQuery { @CheckForNull public Date createdAfter() { - return createdAfter; + return (createdAfter == null ? null : new Date(createdAfter.getTime())); } @CheckForNull public Date createdBefore() { - return createdBefore; + return (createdBefore == null ? null : new Date(createdBefore.getTime())); } @CheckForNull diff --git a/sonar-server/src/main/java/org/sonar/server/issue/InternalRubyIssueService.java b/sonar-server/src/main/java/org/sonar/server/issue/InternalRubyIssueService.java index 1432fddeab4..702da148d3d 100644 --- a/sonar-server/src/main/java/org/sonar/server/issue/InternalRubyIssueService.java +++ b/sonar-server/src/main/java/org/sonar/server/issue/InternalRubyIssueService.java @@ -62,6 +62,10 @@ import java.util.Map; */ public class InternalRubyIssueService implements ServerComponent { + private final static String ID_PARAM = "id"; + private final static String NAME_PARAM = "name"; + private final static String DESCRIPTION_PARAM = "description"; + private final IssueService issueService; private final IssueCommentService commentService; private final IssueChangelogService changelogService; @@ -285,13 +289,13 @@ public class InternalRubyIssueService implements ServerComponent { Result createActionPlanResult(Map parameters, @Nullable DefaultActionPlan existingActionPlan) { Result result = Result.of(); - String name = parameters.get("name"); - String description = parameters.get("description"); + String name = parameters.get(NAME_PARAM); + String description = parameters.get(DESCRIPTION_PARAM); String deadLineParam = parameters.get("deadLine"); String projectParam = parameters.get("project"); - checkMandatorySizeParameter(name, "name", 200, result); - checkOptionalSizeParameter(description, "description", 1000, result); + checkMandatorySizeParameter(name, NAME_PARAM, 200, result); + checkOptionalSizeParameter(description, DESCRIPTION_PARAM, 1000, result); // Can only set project on creation if (existingActionPlan == null) { @@ -511,22 +515,22 @@ public class InternalRubyIssueService implements ServerComponent { @VisibleForTesting DefaultIssueFilter createIssueFilterResult(Map params, boolean checkId, boolean checkUser) { - String id = params.get("id"); - String name = params.get("name"); - String description = params.get("description"); + String id = params.get(ID_PARAM); + String name = params.get(NAME_PARAM); + String description = params.get(DESCRIPTION_PARAM); String data = params.get("data"); String user = params.get("user"); Boolean sharedParam = RubyUtils.toBoolean(params.get("shared")); boolean shared = sharedParam != null ? sharedParam : false; if (checkId) { - checkMandatoryParameter(id, "id"); + checkMandatoryParameter(id, ID_PARAM); } if (checkUser) { checkMandatoryParameter(user, "user"); } - checkMandatorySizeParameter(name, "name", 100); - checkOptionalSizeParameter(description, "description", 4000); + checkMandatorySizeParameter(name, NAME_PARAM, 100); + checkOptionalSizeParameter(description, DESCRIPTION_PARAM, 4000); DefaultIssueFilter defaultIssueFilter = DefaultIssueFilter.create(name) .setDescription(description) diff --git a/sonar-server/src/test/java/org/sonar/server/exceptions/HttpExceptionTest.java b/sonar-server/src/test/java/org/sonar/server/exceptions/HttpExceptionTest.java new file mode 100644 index 00000000000..7b6619bab65 --- /dev/null +++ b/sonar-server/src/test/java/org/sonar/server/exceptions/HttpExceptionTest.java @@ -0,0 +1,57 @@ +/* + * SonarQube, open source software quality management tool. + * Copyright (C) 2008-2013 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.server.exceptions; + +import org.junit.Test; + +import static org.fest.assertions.Assertions.assertThat; + +public class HttpExceptionTest { + + @Test + public void should_create_exception_with_status(){ + HttpException exception = new HttpException(400); + assertThat(exception.httpCode()).isEqualTo(400); + } + + @Test + public void should_create_exception_with_status_and_message(){ + HttpException exception = new HttpException(404, "Not found"); + assertThat(exception.httpCode()).isEqualTo(404); + assertThat(exception.getMessage()).isEqualTo("Not found"); + } + + @Test + public void should_create_exception_with_status_and_l10n_message_with_param(){ + HttpException exception = new HttpException(404, null, "key", new String[]{"value"}); + assertThat(exception.httpCode()).isEqualTo(404); + assertThat(exception.l10nKey()).isEqualTo("key"); + assertThat(exception.l10nParams()).containsOnly("value"); + } + + @Test + public void should_create_exception_with_status_and_l10n_message_without_param(){ + HttpException exception = new HttpException(404, null, "key", null); + assertThat(exception.httpCode()).isEqualTo(404); + assertThat(exception.l10nKey()).isEqualTo("key"); + assertThat(exception.l10nParams()).isEmpty(); + } +} diff --git a/sonar-ws-client/src/test/java/org/sonar/wsclient/services/ResourceQueryTest.java b/sonar-ws-client/src/test/java/org/sonar/wsclient/services/ResourceQueryTest.java index 4cbe8a78d38..17955c9cdd8 100644 --- a/sonar-ws-client/src/test/java/org/sonar/wsclient/services/ResourceQueryTest.java +++ b/sonar-ws-client/src/test/java/org/sonar/wsclient/services/ResourceQueryTest.java @@ -21,34 +21,32 @@ package org.sonar.wsclient.services; import org.junit.Test; -import static org.hamcrest.Matchers.is; -import static org.hamcrest.core.IsNull.nullValue; -import static org.junit.Assert.assertThat; +import static org.fest.assertions.Assertions.assertThat; public class ResourceQueryTest extends QueryTestCase { @Test public void resource() { ResourceQuery query = new ResourceQuery("org.foo:bar"); - assertThat(query.getUrl(), is("/api/resources?resource=org.foo%3Abar&verbose=false&")); - assertThat(query.getResourceKeyOrId(), is("org.foo:bar")); - assertThat(query.isVerbose(), is(false)); + assertThat(query.getUrl()).isEqualTo(("/api/resources?resource=org.foo%3Abar&verbose=false&")); + assertThat(query.getResourceKeyOrId()).isEqualTo(("org.foo:bar")); + assertThat(query.isVerbose()).isEqualTo((false)); } @Test public void resourceByLanguages() { ResourceQuery query = new ResourceQuery("org.foo:bar").setLanguages("java,php"); - assertThat(query.getUrl(), is("/api/resources?resource=org.foo%3Abar&languages=java%2Cphp&verbose=false&")); - assertThat(query.getResourceKeyOrId(), is("org.foo:bar")); + assertThat(query.getUrl()).isEqualTo(("/api/resources?resource=org.foo%3Abar&languages=java%2Cphp&verbose=false&")); + assertThat(query.getResourceKeyOrId()).isEqualTo(("org.foo:bar")); } @Test public void measures() { ResourceQuery query = new ResourceQuery(); query.setMetrics("loc", "coverage", "lines"); - assertThat(query.getUrl(), is("/api/resources?metrics=loc,coverage,lines&verbose=false&")); - assertThat(query.getResourceKeyOrId(), nullValue()); - assertThat(query.getMetrics(), is(new String[] { "loc", "coverage", "lines" })); + assertThat(query.getUrl()).isEqualTo(("/api/resources?metrics=loc,coverage,lines&verbose=false&")); + assertThat(query.getResourceKeyOrId()).isNull(); + assertThat(query.getMetrics()).isEqualTo((new String[]{"loc", "coverage", "lines"})); } @Test @@ -56,14 +54,14 @@ public class ResourceQueryTest extends QueryTestCase { ResourceQuery query = new ResourceQuery(); query.setIncludeTrends(true); - assertThat(query.getUrl(), is("/api/resources?includetrends=true&verbose=false&")); + assertThat(query.getUrl()).isEqualTo(("/api/resources?includetrends=true&verbose=false&")); } @Test public void measuresOnRules() { ResourceQuery query = new ResourceQuery().setMetrics("violations"); query.setRules("ruleA", "ruleB"); - assertThat(query.getUrl(), is("/api/resources?metrics=violations&rules=ruleA,ruleB&verbose=false&")); + assertThat(query.getUrl()).isEqualTo(("/api/resources?metrics=violations&rules=ruleA,ruleB&verbose=false&")); } @Test @@ -71,13 +69,29 @@ public class ResourceQueryTest extends QueryTestCase { ResourceQuery query = new ResourceQuery().setMetrics("violations"); query.setRuleSeverities("MAJOR", "MINOR"); - assertThat(query.getUrl(), is("/api/resources?metrics=violations&rule_priorities=MAJOR,MINOR&verbose=false&")); + assertThat(query.getUrl()).isEqualTo(("/api/resources?metrics=violations&rule_priorities=MAJOR,MINOR&verbose=false&")); } @Test - public void build() { + public void should_create_query() { ResourceQuery query = ResourceQuery.createForMetrics("org.foo", "ncloc", "lines"); - assertThat(query.getResourceKeyOrId(), is("org.foo")); - assertThat(query.getMetrics(), is(new String[] { "ncloc", "lines" })); + assertThat(query.getResourceKeyOrId()).isEqualTo(("org.foo")); + assertThat(query.getMetrics()).isEqualTo((new String[]{"ncloc", "lines"})); + } + + @Test + public void should_create_query_from_resource() { + ResourceQuery query = ResourceQuery.createForResource(new Resource().setId(1), "ncloc"); + assertThat(query.getResourceKeyOrId()).isEqualTo("1"); + assertThat(query.getUrl()).isEqualTo("/api/resources?resource=1&metrics=ncloc&verbose=false&"); + } + + @Test + public void should_not_create_query_from_resource_without_id() { + try { + ResourceQuery.createForResource(new Resource()); + } catch (Exception e) { + assertThat(e).isInstanceOf(IllegalArgumentException.class); + } } } diff --git a/sonar-ws-client/src/test/java/org/sonar/wsclient/services/TimeMachineQueryTest.java b/sonar-ws-client/src/test/java/org/sonar/wsclient/services/TimeMachineQueryTest.java index af83f183e9b..d089f427991 100644 --- a/sonar-ws-client/src/test/java/org/sonar/wsclient/services/TimeMachineQueryTest.java +++ b/sonar-ws-client/src/test/java/org/sonar/wsclient/services/TimeMachineQueryTest.java @@ -19,18 +19,17 @@ */ package org.sonar.wsclient.services; -import static org.hamcrest.Matchers.is; -import static org.junit.Assert.assertThat; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import org.sonar.wsclient.JdkUtils; import java.text.ParseException; import java.text.SimpleDateFormat; import java.util.Date; import java.util.TimeZone; -import org.junit.After; -import org.junit.Before; -import org.junit.Test; -import org.sonar.wsclient.JdkUtils; +import static org.fest.assertions.Assertions.assertThat; public class TimeMachineQueryTest extends QueryTestCase { @@ -49,18 +48,31 @@ public class TimeMachineQueryTest extends QueryTestCase { } @Test - public void shouldGetUrl() { + public void should_get_url() { TimeMachineQuery query = TimeMachineQuery.createForMetrics("12345", "ncloc", "coverage"); - assertThat(query.getUrl(), is("/api/timemachine?resource=12345&metrics=ncloc,coverage&")); + assertThat(query.getUrl()).isEqualTo("/api/timemachine?resource=12345&metrics=ncloc,coverage&"); } @Test - public void shouldSetPeriod() throws ParseException { + public void should_set_period() throws ParseException { Date from = new SimpleDateFormat("yyyy-MM-dd").parse("2010-02-18"); Date to = new SimpleDateFormat("yyyy-MM-dd HH:mm").parse("2010-03-25 14:59"); TimeMachineQuery query = TimeMachineQuery.createForMetrics("12345", "ncloc").setFrom(from).setTo(to); - assertThat( - query.getUrl(), - is("/api/timemachine?resource=12345&metrics=ncloc&fromDateTime=2010-02-18T00%3A00%3A00%2B0000&toDateTime=2010-03-25T14%3A59%3A00%2B0000&")); + assertThat(query.getUrl()).isEqualTo( + "/api/timemachine?resource=12345&metrics=ncloc&fromDateTime=2010-02-18T00%3A00%3A00%2B0000&toDateTime=2010-03-25T14%3A59%3A00%2B0000&"); + } + + @Test + public void should_create_query_from_resource() { + TimeMachineQuery query = TimeMachineQuery.createForMetrics(new Resource().setId(1), "ncloc"); + assertThat(query.getUrl()).isEqualTo("/api/timemachine?resource=1&metrics=ncloc&"); } + + @Test + public void should_not_create_query_from_resource_without_id() { + try { + TimeMachineQuery.createForMetrics(new Resource()); + } catch (Exception e) { + assertThat(e).isInstanceOf(IllegalArgumentException.class); + } } } diff --git a/sonar-ws-client/src/test/java/org/sonar/wsclient/services/ViolationQueryTest.java b/sonar-ws-client/src/test/java/org/sonar/wsclient/services/ViolationQueryTest.java index 6da64d96cc1..2d8007bb57b 100644 --- a/sonar-ws-client/src/test/java/org/sonar/wsclient/services/ViolationQueryTest.java +++ b/sonar-ws-client/src/test/java/org/sonar/wsclient/services/ViolationQueryTest.java @@ -19,30 +19,44 @@ */ package org.sonar.wsclient.services; -import static org.hamcrest.Matchers.is; -import static org.junit.Assert.assertThat; - import org.junit.Test; +import static org.fest.assertions.Assertions.assertThat; + public class ViolationQueryTest extends QueryTestCase { @Test - public void resourceViolations() { + public void should_create_query() { ViolationQuery query = ViolationQuery.createForResource("myproject:org.foo:bar"); - assertThat(query.getUrl(), is("/api/violations?resource=myproject%3Aorg.foo%3Abar&")); - assertThat(query.getModelClass().getName(), is(Violation.class.getName())); + assertThat(query.getUrl()).isEqualTo("/api/violations?resource=myproject%3Aorg.foo%3Abar&"); + assertThat(query.getModelClass().getName()).isEqualTo(Violation.class.getName()); } @Test - public void resourceTreeViolations() { + public void should_create_query_tree() { ViolationQuery query = ViolationQuery.createForResource("myproject") - .setDepth(-1) - .setLimit(20) - .setSeverities("MAJOR", "BLOCKER") - .setQualifiers("FIL") - .setRuleKeys("checkstyle:foo", "pmd:bar"); - assertThat( - query.getUrl(), - is("/api/violations?resource=myproject&depth=-1&limit=20&qualifiers=FIL&rules=checkstyle%3Afoo,pmd%3Abar&priorities=MAJOR,BLOCKER&")); + .setDepth(-1) + .setLimit(20) + .setSeverities("MAJOR", "BLOCKER") + .setQualifiers("FIL") + .setRuleKeys("checkstyle:foo", "pmd:bar"); + assertThat(query.getUrl()).isEqualTo( + "/api/violations?resource=myproject&depth=-1&limit=20&qualifiers=FIL&rules=checkstyle%3Afoo,pmd%3Abar&priorities=MAJOR,BLOCKER&"); + } + + @Test + public void should_create_query_from_resource() { + ViolationQuery query = ViolationQuery.createForResource(new Resource().setId(1)); + assertThat(query.getUrl()).isEqualTo("/api/violations?resource=1&"); + assertThat(query.getModelClass().getName()).isEqualTo(Violation.class.getName()); + } + + @Test + public void should_not_create_query_from_resource_without_id() { + try { + ViolationQuery.createForResource(new Resource()); + } catch (Exception e) { + assertThat(e).isInstanceOf(IllegalArgumentException.class); + } } } diff --git a/sonar-ws-client/src/test/java/org/sonar/wsclient/unmarshallers/AuthenticationUnmarshallerTest.java b/sonar-ws-client/src/test/java/org/sonar/wsclient/unmarshallers/AuthenticationUnmarshallerTest.java index f66a507e94f..939d35645ad 100644 --- a/sonar-ws-client/src/test/java/org/sonar/wsclient/unmarshallers/AuthenticationUnmarshallerTest.java +++ b/sonar-ws-client/src/test/java/org/sonar/wsclient/unmarshallers/AuthenticationUnmarshallerTest.java @@ -23,6 +23,7 @@ import org.junit.Test; import org.sonar.wsclient.services.Authentication; import static org.fest.assertions.Assertions.assertThat; +import static org.fest.assertions.Fail.fail; public class AuthenticationUnmarshallerTest extends UnmarshallerTestCase { @Test @@ -38,4 +39,21 @@ public class AuthenticationUnmarshallerTest extends UnmarshallerTestCase { assertThat(authentication.isValid()).isFalse(); } + + @Test + public void should_unmarshall_empty_authentication() { + Authentication authentication = new AuthenticationUnmarshaller().toModel("{}"); + + assertThat(authentication.isValid()).isFalse(); + } + + @Test + public void should_not_umarshall_authentication_list() { + try { + new AuthenticationUnmarshaller().toModels("[{\"valid\":true},{\"valid\":true}]"); + fail(); + } catch (Exception e) { + assertThat(e).isInstanceOf(UnsupportedOperationException.class); + } + } } -- 2.39.5