From f8a905f962697cee341f9b1c266844091b8d3be7 Mon Sep 17 00:00:00 2001 From: Julien Lancelot Date: Wed, 1 Feb 2017 12:12:25 +0100 Subject: [PATCH] SONAR-8719 Do not hide original exception when failing to write JSON errors --- .../org/sonar/server/ws/WebServiceEngine.java | 10 +- .../sonar/server/ws/WebServiceEngineTest.java | 123 ++++++++---------- .../org/sonar/api/utils/text/JsonWriter.java | 3 +- 3 files changed, 64 insertions(+), 72 deletions(-) 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 98cdcbd332e..ba72f45cf59 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 @@ -19,6 +19,7 @@ */ package org.sonar.server.ws; +import com.google.common.base.Throwables; import java.io.OutputStreamWriter; import java.nio.charset.StandardCharsets; import java.util.List; @@ -139,15 +140,14 @@ public class WebServiceEngine implements LocalConnector, Startable { } stream.setStatus(status); stream.setMediaType(MediaTypes.JSON); - JsonWriter json = JsonWriter.of(new OutputStreamWriter(stream.output(), StandardCharsets.UTF_8)); - try { + try (JsonWriter json = JsonWriter.of(new OutputStreamWriter(stream.output(), StandardCharsets.UTF_8))) { json.beginObject(); errors.writeJson(json, i18n); json.endObject(); - } finally { - // TODO if close() fails, the runtime exception should not hide the potential exception raised in the try block. - json.close(); + } catch (Exception e) { + // Do not hide the potential exception raised in the try block. + Throwables.propagate(e); } } 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 136d4ffc18a..d4869eb1044 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 @@ -28,20 +28,22 @@ import org.junit.After; import org.junit.Before; import org.junit.Rule; import org.junit.Test; +import org.junit.rules.ExpectedException; import org.sonar.api.i18n.I18n; -import org.sonar.api.server.ws.Request; -import org.sonar.api.server.ws.RequestHandler; import org.sonar.api.server.ws.Response; import org.sonar.api.server.ws.WebService; import org.sonar.api.server.ws.internal.ValidatingRequest; import org.sonar.api.utils.log.LogTester; import org.sonar.api.utils.log.LoggerLevel; +import org.sonar.api.utils.text.JsonWriter; import org.sonar.server.exceptions.BadRequestException; import org.sonar.server.exceptions.Errors; import org.sonar.server.exceptions.Message; import org.sonarqube.ws.MediaTypes; import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Matchers.any; +import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -50,6 +52,9 @@ public class WebServiceEngineTest { @Rule public LogTester logTester = new LogTester(); + @Rule + public ExpectedException expectedException = ExpectedException.none(); + private I18n i18n = mock(I18n.class); private WebServiceEngine underTest = new WebServiceEngine(new WebService[] {new SystemWs()}, i18n); @@ -302,6 +307,16 @@ public class WebServiceEngineTest { assertThat(response.stream().mediaType()).isEqualTo(MediaTypes.JSON); } + @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"); + DumbResponse response = new DumbResponse(); + + expectedException.expect(IllegalArgumentException.class); + expectedException.expectMessage("Error!"); + underTest.execute(request, response); + } + @Test public void should_handle_headers() { DumbResponse response = new DumbResponse(); @@ -331,97 +346,73 @@ public class WebServiceEngineTest { public void define(Context context) { NewController newController = context.createController("api/system"); createNewDefaultAction(newController, "health") - .setHandler(new RequestHandler() { - @Override - public void handle(Request request, Response response) { - try { - response.stream().output().write("good".getBytes()); - } catch (IOException e) { - throw new IllegalStateException(e); - } + .setHandler((request, response) -> { + try { + response.stream().output().write("good".getBytes()); + } catch (IOException e) { + throw new IllegalStateException(e); } }); createNewDefaultAction(newController, "ping") .setPost(true) - .setHandler(new RequestHandler() { - @Override - public void handle(Request request, Response response) { - try { - response.stream().output().write("pong".getBytes()); - } catch (IOException e) { - throw new IllegalStateException(e); - } + .setHandler((request, response) -> { + try { + response.stream().output().write("pong".getBytes()); + } catch (IOException e) { + throw new IllegalStateException(e); } }); createNewDefaultAction(newController, "fail") - .setHandler(new RequestHandler() { - @Override - public void handle(Request request, Response response) { - throw new IllegalStateException("Unexpected"); - } + .setHandler((request, response) -> { + throw new IllegalStateException("Unexpected"); }); createNewDefaultAction(newController, "fail_with_i18n_message") - .setHandler(new RequestHandler() { - @Override - public void handle(Request request, Response response) { - throw new BadRequestException("bad.request.reason", 0); - } + .setHandler((request, response) -> { + throw new BadRequestException("bad.request.reason", 0); }); createNewDefaultAction(newController, "fail_with_multiple_messages") .createParam("count", "Number of error messages to generate") - .setHandler(new RequestHandler() { - @Override - public void handle(Request request, Response response) { - Errors errors = new Errors(); - for (int count = 0; count < Integer.valueOf(request.param("count")); count++) { - errors.add(Message.of("Bad request reason #" + count)); - } - throw new BadRequestException(errors); + .setHandler((request, response) -> { + Errors errors = new Errors(); + for (int count = 0; count < Integer.valueOf(request.param("count")); count++) { + errors.add(Message.of("Bad request reason #" + count)); } + throw new BadRequestException(errors); }); createNewDefaultAction(newController, "fail_with_multiple_i18n_messages") .createParam("count", "Number of error messages to generate") - .setHandler(new RequestHandler() { - @Override - public void handle(Request request, Response response) { - Errors errors = new Errors(); - for (int count = 0; count < Integer.valueOf(request.param("count")); count++) { - errors.add(Message.of("bad.request.reason", count)); - } - throw new BadRequestException(errors); + .setHandler((request, response) -> { + Errors errors = new Errors(); + for (int count = 0; count < Integer.valueOf(request.param("count")); count++) { + errors.add(Message.of("bad.request.reason", count)); } + throw new BadRequestException(errors); }); - createNewDefaultAction(newController, "alive") - .setHandler(new RequestHandler() { - @Override - public void handle(Request request, Response response) { - response.noContent(); - } + createNewDefaultAction(newController, "fail_to_write_errors") + .setHandler((request, response) -> { + Errors errors = mock(Errors.class); + // Try to simulate an error when generating JSON errors + doThrow(new IllegalArgumentException("Error!")).when(errors).writeJson(any(JsonWriter.class), any(I18n.class)); + throw new BadRequestException(errors); }); + createNewDefaultAction(newController, "alive") + .setHandler((request, response) -> response.noContent()); createNewDefaultAction(newController, "fail_with_undeclared_parameter") - .setHandler(new RequestHandler() { - @Override - public void handle(Request request, Response response) { - response.newJsonWriter().prop("unknown", request.param("unknown")); - } - }); + .setHandler((request, response) -> response.newJsonWriter().prop("unknown", request.param("unknown"))); // parameter "message" is required but not "author" NewAction print = createNewDefaultAction(newController, "print"); print.createParam("message").setDescription("required message").setRequired(true); print.createParam("author").setDescription("optional author").setDefaultValue("-"); print.createParam("format").setDescription("optional format").setPossibleValues("json", "xml"); - print.setHandler(new RequestHandler() { - @Override - public void handle(Request request, Response response) { - try { - request.param("format"); - IOUtils.write( - request.mandatoryParam("message") + " by " + request.param("author", "nobody"), response.stream().output()); - } catch (IOException e) { - throw new IllegalStateException(e); - } + print.setHandler((request, response) -> { + try { + request.param("format"); + IOUtils.write( + request.mandatoryParam("message") + " by " + request.param("author", "nobody"), response.stream().output()); + } catch (IOException e) { + throw new IllegalStateException(e); } }); diff --git a/sonar-plugin-api/src/main/java/org/sonar/api/utils/text/JsonWriter.java b/sonar-plugin-api/src/main/java/org/sonar/api/utils/text/JsonWriter.java index b40e8e1dfb0..a098a2388a4 100644 --- a/sonar-plugin-api/src/main/java/org/sonar/api/utils/text/JsonWriter.java +++ b/sonar-plugin-api/src/main/java/org/sonar/api/utils/text/JsonWriter.java @@ -59,7 +59,7 @@ import org.sonar.api.utils.DateUtils; * * @since 4.2 */ -public class JsonWriter { +public class JsonWriter implements AutoCloseable { private final com.google.gson.stream.JsonWriter stream; private boolean serializeEmptyStrings; @@ -379,6 +379,7 @@ public class JsonWriter { /** * @throws org.sonar.api.utils.text.WriterException on any failure */ + @Override public void close() { try { stream.close(); -- 2.39.5