From 187b578649d987229905533bc7496c6ab3e6785b Mon Sep 17 00:00:00 2001 From: Julien Lancelot Date: Wed, 1 Mar 2017 12:33:01 +0100 Subject: [PATCH] SONAR-8460 Remove Errors --- .../exceptions/BadRequestException.java | 14 ++--- .../org/sonar/server/exceptions/Errors.java | 63 ------------------- .../qualityprofile/BulkChangeResult.java | 14 ++--- .../server/qualityprofile/QProfileReset.java | 2 +- .../server/qualityprofile/RuleActivator.java | 4 +- .../ws/BulkRuleActivationActions.java | 4 +- .../org/sonar/server/rule/RuleCreator.java | 4 +- .../org/sonar/server/ws/WebServiceEngine.java | 14 ++--- .../sonar/server/email/ws/SendActionTest.java | 3 +- .../exceptions/BadRequestExceptionTest.java | 4 +- .../RuleActivatorMediumTest.java | 4 +- .../sonar/server/user/UserUpdaterTest.java | 2 +- .../sonar/server/ws/WebServiceEngineTest.java | 17 +++-- 13 files changed, 37 insertions(+), 112 deletions(-) delete mode 100644 server/sonar-server/src/main/java/org/sonar/server/exceptions/Errors.java diff --git a/server/sonar-server/src/main/java/org/sonar/server/exceptions/BadRequestException.java b/server/sonar-server/src/main/java/org/sonar/server/exceptions/BadRequestException.java index e6d4ae4b302..5bdba7c6638 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/exceptions/BadRequestException.java +++ b/server/sonar-server/src/main/java/org/sonar/server/exceptions/BadRequestException.java @@ -21,7 +21,6 @@ package org.sonar.server.exceptions; import com.google.common.base.MoreObjects; import java.util.List; -import java.util.stream.Collectors; import static com.google.common.base.Preconditions.checkArgument; import static java.net.HttpURLConnection.HTTP_BAD_REQUEST; @@ -32,23 +31,24 @@ import static java.util.Arrays.asList; */ public class BadRequestException extends ServerException { - private final transient Errors errors; + private final transient List errors; - private BadRequestException(Errors e) { - super(HTTP_BAD_REQUEST, e.messages().get(0).getMessage()); - this.errors = e; + private BadRequestException(List errors) { + super(HTTP_BAD_REQUEST, errors.get(0)); + this.errors = errors; } public static BadRequestException create(List errorMessages) { checkArgument(!errorMessages.isEmpty(), "At least one error message is required"); - return new BadRequestException(new Errors().add(errorMessages.stream().map(Message::of).collect(Collectors.toList()))); + checkArgument(errorMessages.stream().noneMatch(message -> message == null || message.isEmpty()), "Message cannot be empty"); + return new BadRequestException(errorMessages); } public static BadRequestException create(String... errorMessages) { return create(asList(errorMessages)); } - public Errors errors() { + public List errors() { return errors; } diff --git a/server/sonar-server/src/main/java/org/sonar/server/exceptions/Errors.java b/server/sonar-server/src/main/java/org/sonar/server/exceptions/Errors.java deleted file mode 100644 index e0045390bbb..00000000000 --- a/server/sonar-server/src/main/java/org/sonar/server/exceptions/Errors.java +++ /dev/null @@ -1,63 +0,0 @@ -/* - * SonarQube - * Copyright (C) 2009-2017 SonarSource SA - * mailto:info AT sonarsource DOT com - * - * This program is free software; you can redistribute it and/or - * modify it under the terms of the GNU Lesser General Public - * License as published by the Free Software Foundation; either - * version 3 of the License, or (at your option) any later version. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - * Lesser General Public License for more details. - * - * You should have received a copy of the GNU Lesser General Public License - * along with this program; if not, write to the Free Software Foundation, - * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. - */ -package org.sonar.server.exceptions; - -import com.google.common.base.MoreObjects; -import com.google.common.collect.Lists; -import java.util.Collection; -import java.util.Collections; -import java.util.List; - -public class Errors { - - private final List messages = Lists.newArrayList(); - - public Errors() { - } - - public Errors add(Errors errors) { - this.messages.addAll(errors.messages()); - return this; - } - - public Errors add(Collection messages) { - for (Message message : messages) { - add(message); - } - return this; - } - - public Errors add(Message message, Message... others) { - messages.add(message); - Collections.addAll(messages, others); - return this; - } - - public List messages() { - return messages; - } - - @Override - public String toString() { - return MoreObjects.toStringHelper(this) - .add("messages", messages) - .toString(); - } -} diff --git a/server/sonar-server/src/main/java/org/sonar/server/qualityprofile/BulkChangeResult.java b/server/sonar-server/src/main/java/org/sonar/server/qualityprofile/BulkChangeResult.java index 75c686b8972..272e9f90f20 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/qualityprofile/BulkChangeResult.java +++ b/server/sonar-server/src/main/java/org/sonar/server/qualityprofile/BulkChangeResult.java @@ -20,19 +20,17 @@ package org.sonar.server.qualityprofile; import com.google.common.collect.Lists; -import org.sonar.db.qualityprofile.QualityProfileDto; -import org.sonar.server.exceptions.Errors; - -import javax.annotation.CheckForNull; -import javax.annotation.Nullable; - +import java.util.ArrayList; import java.util.Collection; import java.util.List; +import javax.annotation.CheckForNull; +import javax.annotation.Nullable; +import org.sonar.db.qualityprofile.QualityProfileDto; public class BulkChangeResult { private final QualityProfileDto profile; - private final Errors errors = new Errors(); + private final List errors = new ArrayList<>(); private int succeeded = 0; private int failed = 0; private final List changes = Lists.newArrayList(); @@ -50,7 +48,7 @@ public class BulkChangeResult { return profile; } - public Errors getErrors() { + public List getErrors() { return errors; } diff --git a/server/sonar-server/src/main/java/org/sonar/server/qualityprofile/QProfileReset.java b/server/sonar-server/src/main/java/org/sonar/server/qualityprofile/QProfileReset.java index 9f37b9d2aa7..009dfe692b7 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/qualityprofile/QProfileReset.java +++ b/server/sonar-server/src/main/java/org/sonar/server/qualityprofile/QProfileReset.java @@ -141,7 +141,7 @@ public class QProfileReset { result.addChanges(changes); } catch (BadRequestException e) { result.incrementFailed(); - result.getErrors().add(e.errors()); + result.getErrors().addAll(e.errors()); } } diff --git a/server/sonar-server/src/main/java/org/sonar/server/qualityprofile/RuleActivator.java b/server/sonar-server/src/main/java/org/sonar/server/qualityprofile/RuleActivator.java index ed7917e5331..745cdf8f31c 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/qualityprofile/RuleActivator.java +++ b/server/sonar-server/src/main/java/org/sonar/server/qualityprofile/RuleActivator.java @@ -422,7 +422,7 @@ public class RuleActivator { } catch (BadRequestException e) { // other exceptions stop the bulk activation result.incrementFailed(); - result.getErrors().add(e.errors()); + result.getErrors().addAll(e.errors()); } } dbSession.commit(); @@ -450,7 +450,7 @@ public class RuleActivator { } catch (BadRequestException e) { // other exceptions stop the bulk activation result.incrementFailed(); - result.getErrors().add(e.errors()); + result.getErrors().addAll(e.errors()); } } dbSession.commit(); diff --git a/server/sonar-server/src/main/java/org/sonar/server/qualityprofile/ws/BulkRuleActivationActions.java b/server/sonar-server/src/main/java/org/sonar/server/qualityprofile/ws/BulkRuleActivationActions.java index 58c88134446..2ee4f0d9dff 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/qualityprofile/ws/BulkRuleActivationActions.java +++ b/server/sonar-server/src/main/java/org/sonar/server/qualityprofile/ws/BulkRuleActivationActions.java @@ -26,8 +26,6 @@ import org.sonar.api.server.ws.Request; import org.sonar.api.server.ws.Response; import org.sonar.api.server.ws.WebService; import org.sonar.api.utils.text.JsonWriter; -import org.sonar.core.util.stream.Collectors; -import org.sonar.server.exceptions.Message; import org.sonar.server.qualityprofile.BulkChangeResult; import org.sonar.server.qualityprofile.QProfileService; import org.sonar.server.rule.ws.RuleQueryFactory; @@ -110,7 +108,7 @@ public class BulkRuleActivationActions { JsonWriter json = response.newJsonWriter().beginObject(); json.prop("succeeded", result.countSucceeded()); json.prop("failed", result.countFailed()); - writeErrors(json, result.getErrors().messages().stream().map(Message::getMessage).collect(Collectors.toList())); + writeErrors(json, result.getErrors()); json.endObject().close(); } diff --git a/server/sonar-server/src/main/java/org/sonar/server/rule/RuleCreator.java b/server/sonar-server/src/main/java/org/sonar/server/rule/RuleCreator.java index 60e86186edb..0a410d6d259 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/rule/RuleCreator.java +++ b/server/sonar-server/src/main/java/org/sonar/server/rule/RuleCreator.java @@ -32,14 +32,12 @@ import org.sonar.api.rule.Severity; import org.sonar.api.server.ServerSide; import org.sonar.api.server.rule.RuleParamType; import org.sonar.api.utils.System2; -import org.sonar.core.util.stream.Collectors; import org.sonar.db.DbClient; import org.sonar.db.DbSession; import org.sonar.db.rule.RuleDto; import org.sonar.db.rule.RuleDto.Format; import org.sonar.db.rule.RuleParamDto; import org.sonar.server.exceptions.BadRequestException; -import org.sonar.server.exceptions.Message; import org.sonar.server.rule.index.RuleIndexer; import org.sonar.server.util.TypeValidations; @@ -117,7 +115,7 @@ public class RuleCreator { try { validateParam(ruleParam, newRule.parameter(ruleParam.getName())); } catch (BadRequestException validationError) { - errors.addAll(validationError.errors().messages().stream().map(Message::getMessage).collect(Collectors.toList())); + errors.addAll(validationError.errors()); } } checkRequest(errors.isEmpty(), errors); diff --git a/server/sonar-server/src/main/java/org/sonar/server/ws/WebServiceEngine.java b/server/sonar-server/src/main/java/org/sonar/server/ws/WebServiceEngine.java index 16db82df676..ea1bc1d0d29 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/ws/WebServiceEngine.java +++ b/server/sonar-server/src/main/java/org/sonar/server/ws/WebServiceEngine.java @@ -36,15 +36,13 @@ import org.sonar.api.server.ws.internal.ValidatingRequest; import org.sonar.api.utils.log.Logger; import org.sonar.api.utils.log.Loggers; import org.sonar.api.utils.text.JsonWriter; -import org.sonar.core.util.stream.Collectors; import org.sonar.server.exceptions.BadRequestException; -import org.sonar.server.exceptions.Errors; -import org.sonar.server.exceptions.Message; import org.sonar.server.exceptions.ServerException; import org.sonarqube.ws.MediaTypes; import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Strings.isNullOrEmpty; +import static java.util.Collections.singletonList; import static org.apache.commons.lang.StringUtils.substring; import static org.apache.commons.lang.StringUtils.substringAfterLast; import static org.apache.commons.lang.StringUtils.substringBeforeLast; @@ -104,11 +102,11 @@ public class WebServiceEngine implements LocalConnector, Startable { verifyRequest(action, request); action.handler().handle(request, response); } catch (IllegalArgumentException e) { - sendErrors(response, 400, new Errors().add(Message.of(e.getMessage()))); + sendErrors(response, 400, singletonList(e.getMessage())); } catch (BadRequestException e) { sendErrors(response, 400, e.errors()); } catch (ServerException e) { - sendErrors(response, e.httpCode(), new Errors().add(Message.of(e.getMessage()))); + sendErrors(response, e.httpCode(), singletonList(e.getMessage())); } catch (Exception e) { Response.Stream stream = response.stream(); if (stream instanceof ServletResponse.ServletStream && ((ServletResponse.ServletStream) stream).response().isCommitted()) { @@ -119,7 +117,7 @@ public class WebServiceEngine implements LocalConnector, Startable { LOGGER.error("Fail to process request " + request, e); // Sending exception message into response is a vulnerability. Error must be // displayed only in logs. - sendErrors(response, 500, new Errors().add(Message.of("An error has occurred. Please contact your administrator"))); + sendErrors(response, 500, singletonList("An error has occurred. Please contact your administrator")); } } @@ -131,7 +129,7 @@ public class WebServiceEngine implements LocalConnector, Startable { return controller == null ? null : controller.action(actionKey); } - private static void sendErrors(Response response, int status, Errors errors) { + private static void sendErrors(Response response, int status, List errors) { Response.Stream stream = response.stream(); if (stream instanceof ServletResponse.ServletStream) { ((ServletResponse.ServletStream) stream).reset(); @@ -141,7 +139,7 @@ public class WebServiceEngine implements LocalConnector, Startable { try (JsonWriter json = JsonWriter.of(new OutputStreamWriter(stream.output(), StandardCharsets.UTF_8))) { json.beginObject(); - writeErrors(json, errors.messages().stream().map(Message::getMessage).collect(Collectors.toList())); + writeErrors(json, errors); json.endObject(); } catch (Exception e) { // Do not hide the potential exception raised in the try block. diff --git a/server/sonar-server/src/test/java/org/sonar/server/email/ws/SendActionTest.java b/server/sonar-server/src/test/java/org/sonar/server/email/ws/SendActionTest.java index a0885176a5f..6575221895d 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/email/ws/SendActionTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/email/ws/SendActionTest.java @@ -27,7 +27,6 @@ import org.junit.rules.ExpectedException; import org.sonar.api.server.ws.WebService; import org.sonar.server.exceptions.BadRequestException; import org.sonar.server.exceptions.ForbiddenException; -import org.sonar.server.exceptions.Message; import org.sonar.server.notification.email.EmailNotificationChannel; import org.sonar.server.tester.UserSessionRule; import org.sonar.server.ws.TestRequest; @@ -111,7 +110,7 @@ public class SendActionTest { executeRequest("john@doo.com", "Test Message from SonarQube", "This is a test message from SonarQube at http://localhost:9000"); fail(); } catch (BadRequestException e) { - assertThat(e.errors().messages()).extracting(Message::getMessage).containsExactly( + assertThat(e.errors()).containsExactly( "root cause", "parent cause", "child cause", "last message"); } } diff --git a/server/sonar-server/src/test/java/org/sonar/server/exceptions/BadRequestExceptionTest.java b/server/sonar-server/src/test/java/org/sonar/server/exceptions/BadRequestExceptionTest.java index 307e6c84815..eb9f67d9d39 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/exceptions/BadRequestExceptionTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/exceptions/BadRequestExceptionTest.java @@ -42,14 +42,14 @@ public class BadRequestExceptionTest { public void create_exception_from_list() throws Exception { BadRequestException underTest = BadRequestException.create(asList("error1", "error2")); - assertThat(underTest.errors().messages().stream().map(Message::getMessage)).containsOnly("error1", "error2"); + assertThat(underTest.errors()).containsOnly("error1", "error2"); } @Test public void create_exception_from_var_args() throws Exception { BadRequestException underTest = BadRequestException.create("error1", "error2"); - assertThat(underTest.errors().messages().stream().map(Message::getMessage)).containsOnly("error1", "error2"); + assertThat(underTest.errors()).containsOnly("error1", "error2"); } @Test diff --git a/server/sonar-server/src/test/java/org/sonar/server/qualityprofile/RuleActivatorMediumTest.java b/server/sonar-server/src/test/java/org/sonar/server/qualityprofile/RuleActivatorMediumTest.java index 1fe5a84b2ec..b7ffe8148a6 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/qualityprofile/RuleActivatorMediumTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/qualityprofile/RuleActivatorMediumTest.java @@ -43,7 +43,6 @@ import org.sonar.db.rule.RuleDto; import org.sonar.db.rule.RuleParamDto; import org.sonar.server.es.SearchOptions; import org.sonar.server.exceptions.BadRequestException; -import org.sonar.server.exceptions.Message; import org.sonar.server.qualityprofile.index.ActiveRuleIndex; import org.sonar.server.qualityprofile.index.ActiveRuleIndexer; import org.sonar.server.rule.index.RuleIndex; @@ -495,8 +494,7 @@ public class RuleActivatorMediumTest { activate(activation, XOO_P1_KEY); fail(); } catch (BadRequestException e) { - Message msg = e.errors().messages().get(0); - assertThat(msg.getMessage()).isEqualTo("Value 'foo' must be an integer."); + assertThat(e.getMessage()).isEqualTo("Value 'foo' must be an integer."); verifyZeroActiveRules(XOO_P1_KEY); } } diff --git a/server/sonar-server/src/test/java/org/sonar/server/user/UserUpdaterTest.java b/server/sonar-server/src/test/java/org/sonar/server/user/UserUpdaterTest.java index b444450e595..2b723280d74 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/user/UserUpdaterTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/user/UserUpdaterTest.java @@ -357,7 +357,7 @@ public class UserUpdaterTest { .build()); fail(); } catch (BadRequestException e) { - assertThat(e.errors().messages()).hasSize(3); + assertThat(e.errors()).hasSize(3); } } diff --git a/server/sonar-server/src/test/java/org/sonar/server/ws/WebServiceEngineTest.java b/server/sonar-server/src/test/java/org/sonar/server/ws/WebServiceEngineTest.java index 5da0f2430c1..e3b32235386 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/ws/WebServiceEngineTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/ws/WebServiceEngineTest.java @@ -22,7 +22,6 @@ package org.sonar.server.ws; import java.io.IOException; import java.util.ArrayList; import java.util.List; -import java.util.MissingFormatArgumentException; import javax.servlet.http.HttpServletResponse; import org.apache.catalina.connector.ClientAbortException; import org.apache.commons.io.IOUtils; @@ -39,7 +38,6 @@ import org.sonar.api.utils.log.LoggerLevel; import org.sonar.server.exceptions.BadRequestException; import org.sonarqube.ws.MediaTypes; -import static java.lang.String.format; import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -285,13 +283,15 @@ public class WebServiceEngineTest { } @Test - public void render_real_exception_when_failing_to_write_json_errors() { - ValidatingRequest request = new TestRequest().setMethod("GET").setPath("/api/system/fail_to_write_errors"); + public void does_not_fail_to_render_error_message_having_percent() { + ValidatingRequest request = new TestRequest().setMethod("GET").setPath("/api/system/error_message_having_percent"); DumbResponse response = new DumbResponse(); - expectedException.expect(MissingFormatArgumentException.class); - expectedException.expectMessage("Format specifier '%s'"); underTest.execute(request, response); + + assertThat(response.stream().outputAsString()).isEqualTo("{\"errors\":[{\"msg\":\"this should not fail %s\"}]}"); + assertThat(response.stream().status()).isEqualTo(400); + assertThat(response.stream().mediaType()).isEqualTo(MediaTypes.JSON); } @Test @@ -356,10 +356,9 @@ public class WebServiceEngineTest { } throw BadRequestException.create(errors); }); - createNewDefaultAction(newController, "fail_to_write_errors") + createNewDefaultAction(newController, "error_message_having_percent") .setHandler((request, response) -> { - // Try to simulate an error when generating JSON errors - format("this will fail as no args are given %s"); + throw new IllegalArgumentException("this should not fail %s"); }); createNewDefaultAction(newController, "alive") .setHandler((request, response) -> response.noContent()); -- 2.39.5