From 73a2b7570fa455650c322682a9f47a657dcd7cf4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lo=C3=AFc=20Lambert?= Date: Mon, 11 Mar 2019 17:54:21 +0100 Subject: [PATCH] May add extra fields in ServiceException - may enhance ServiceException with extra fields - limit values to simple objects (nor array nor collection) - add extra fields in error body - add unit tests - note: add test dependency to JSONassert, license Apache 2.0 --- endpoints-framework/build.gradle | 1 + .../api/server/spi/ServiceException.java | 70 ++++++++++++++++++ .../response/RestResponseResultWriter.java | 8 +- .../api/server/spi/ServiceExceptionTest.java | 73 ++++++++++++++++++- .../RestResponseResultWriterTest.java | 50 +++++++++++++ gradle.properties | 1 + 6 files changed, 200 insertions(+), 3 deletions(-) diff --git a/endpoints-framework/build.gradle b/endpoints-framework/build.gradle index 1645cea6..125e5a75 100644 --- a/endpoints-framework/build.gradle +++ b/endpoints-framework/build.gradle @@ -110,6 +110,7 @@ dependencies { testCompile project(':test-utils') testCompile group: 'junit', name: 'junit', version: junitVersion testCompile group: 'org.mockito', name: 'mockito-core', version: mockitoVersion + testCompile group: 'org.skyscreamer', name: 'jsonassert', version: jsonassertVersion testCompile group: 'com.google.truth', name: 'truth', version: truthVersion testCompile group: 'com.google.appengine', name: 'appengine-testing', version: appengineVersion testCompile group: 'com.google.appengine', name: 'appengine-api-stubs', version: appengineVersion diff --git a/endpoints-framework/src/main/java/com/google/api/server/spi/ServiceException.java b/endpoints-framework/src/main/java/com/google/api/server/spi/ServiceException.java index d90f97b8..62d93e79 100644 --- a/endpoints-framework/src/main/java/com/google/api/server/spi/ServiceException.java +++ b/endpoints-framework/src/main/java/com/google/api/server/spi/ServiceException.java @@ -15,6 +15,12 @@ */ package com.google.api.server.spi; +import com.google.common.base.Preconditions; +import com.google.common.collect.ImmutableList; +import com.google.common.flogger.FluentLogger; + +import java.util.Collections; +import java.util.HashMap; import java.util.Map; import java.util.logging.Level; @@ -24,10 +30,16 @@ */ public class ServiceException extends Exception { + private static final FluentLogger logger = FluentLogger.forEnclosingClass(); + + /** Reserved keywords, cannot be set as an extra field name. */ + public static final ImmutableList EXTRA_FIELDS_RESERVED_NAMES = ImmutableList.of("domain", "message", "reason"); + protected final int statusCode; protected final String reason; protected final String domain; protected Level logLevel; + private final Map extraFields = new HashMap<>(); public ServiceException(int statusCode, String statusMessage) { super(statusMessage); @@ -102,6 +114,64 @@ public Map getHeaders() { return null; } + /** + * Associates to this exception an extra field as a field name/value pair. If a field + * with the same name was previously set, the old value is replaced by the specified + * value. + * @return this + * @throws NullPointerException if {@code fieldName} is {@code null}. + * @throws IllegalArgumentException if {@code fieldName} is one of the reserved field + * names {@link #EXTRA_FIELDS_RESERVED_NAMES}. + */ + public ServiceException putExtraField(String fieldName, String value) { + return putExtraFieldInternal(fieldName, value); + } + + /** + * Associates to this exception an extra field as a field name/value pair. If a field + * with the same name was previously set, the old value is replaced by the specified + * value. + * @return this + * @throws NullPointerException if {@code fieldName} is {@code null}. + * @throws IllegalArgumentException if {@code fieldName} is one of the reserved field + * names {@link #EXTRA_FIELDS_RESERVED_NAMES}. + */ + public ServiceException putExtraField(String fieldName, Boolean value) { + return putExtraFieldInternal(fieldName, value); + } + + /** + * Associates to this exception an extra field as a field name/value pair. If a field + * with the same name was previously set, the old value is replaced by the specified + * value. + * @return this + * @throws NullPointerException if {@code fieldName} is {@code null}. + * @throws IllegalArgumentException if {@code fieldName} is one of the reserved field + * names {@link #EXTRA_FIELDS_RESERVED_NAMES}. + */ + public ServiceException putExtraField(String fieldName, Number value) { + return putExtraFieldInternal(fieldName, value); + } + + private ServiceException putExtraFieldInternal(String fieldName, Object value) { + Preconditions.checkNotNull(fieldName); + Preconditions.checkArgument(!EXTRA_FIELDS_RESERVED_NAMES.contains(fieldName), "The field name '%s' is reserved", fieldName); + final Object previousValue = extraFields.put(fieldName, value); + if (previousValue != null) { + logger.atFine().log("Replaced extra field %s: %s => %s", fieldName, previousValue, value); + } + return this; + } + + /** + * Gets the extra fields. The extra fields are returned in an unmodifiable map, + * each field name/value pair is a map entry. The map is empty if no extra field + * has been added. + */ + public final Map getExtraFields() { + return Collections.unmodifiableMap(extraFields); + } + public Level getLogLevel() { return logLevel == null ? getDefaultLoggingLevel(statusCode) : logLevel; } diff --git a/endpoints-framework/src/main/java/com/google/api/server/spi/response/RestResponseResultWriter.java b/endpoints-framework/src/main/java/com/google/api/server/spi/response/RestResponseResultWriter.java index 5fb1732f..9d81a0be 100644 --- a/endpoints-framework/src/main/java/com/google/api/server/spi/response/RestResponseResultWriter.java +++ b/endpoints-framework/src/main/java/com/google/api/server/spi/response/RestResponseResultWriter.java @@ -23,6 +23,7 @@ import com.google.common.base.Strings; import java.io.IOException; +import java.util.Map; import javax.servlet.http.HttpServletResponse; @@ -67,16 +68,19 @@ public void writeError(ServiceException e) throws IOException { e.getReason() : errorMap.getReason(e.getStatusCode()); String domain = !Strings.isNullOrEmpty(e.getDomain()) ? e.getDomain() : errorMap.getDomain(e.getStatusCode()); - write(code, e.getHeaders(), createError(code, reason, domain, e.getMessage())); + write(code, e.getHeaders(), createError(code, reason, domain, e.getMessage(), e.getExtraFields())); } - private Object createError(int code, String reason, String domain, String message) { + private Object createError(int code, String reason, String domain, String message, Map extraFields) { ObjectNode topLevel = objectMapper.createObjectNode(); ObjectNode topError = objectMapper.createObjectNode(); ObjectNode error = objectMapper.createObjectNode(); error.put("domain", domain); error.put("reason", reason); error.put("message", message); + for (Map.Entry extraField : extraFields.entrySet()) { + error.putPOJO(extraField.getKey(), extraField.getValue()); + } topError.set("errors", objectMapper.createArrayNode().add(error)); topError.put("code", code); topError.put("message", message); diff --git a/endpoints-framework/src/test/java/com/google/api/server/spi/ServiceExceptionTest.java b/endpoints-framework/src/test/java/com/google/api/server/spi/ServiceExceptionTest.java index ccc19143..45ed69a6 100644 --- a/endpoints-framework/src/test/java/com/google/api/server/spi/ServiceExceptionTest.java +++ b/endpoints-framework/src/test/java/com/google/api/server/spi/ServiceExceptionTest.java @@ -1,15 +1,27 @@ package com.google.api.server.spi; import static com.google.common.truth.Truth.assertThat; +import static java.lang.Boolean.TRUE; +import com.google.api.server.spi.response.BadRequestException; +import com.google.api.server.spi.response.ConflictException; import com.google.api.server.spi.response.UnauthorizedException; + +import java.util.Map; import java.util.logging.Level; + +import org.junit.Rule; import org.junit.Test; +import org.junit.rules.ExpectedException; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @RunWith(JUnit4.class) public class ServiceExceptionTest { + + @Rule + public ExpectedException expectedException = ExpectedException.none(); + @Test public void testWithLogLevel() { UnauthorizedException ex = new UnauthorizedException(""); @@ -17,4 +29,63 @@ public void testWithLogLevel() { assertThat(ServiceException.withLogLevel(ex, Level.WARNING).getLogLevel()) .isEqualTo(Level.WARNING); } -} \ No newline at end of file + + @Test + public void testExtraFields() { + UnauthorizedException ex = new UnauthorizedException(""); + ex.putExtraField("isAdmin", TRUE) + .putExtraField("userId", Integer.valueOf(12)) + .putExtraField("userName", "John Doe"); + Map extraFields = ex.getExtraFields(); + assertThat(extraFields.size()).isEqualTo(3); + assertThat(extraFields.get("isAdmin")).isEqualTo(TRUE); + assertThat(extraFields.get("userId")).isEqualTo(12); + assertThat(extraFields.get("userName")).isEqualTo("John Doe"); + } + + @Test(expected = NullPointerException.class) + public void testExtraFields_nameNull() { + new BadRequestException("").putExtraField(null, "value not null"); + } + + @Test + public void testExtraFields_valueNull_allowed() { + UnauthorizedException ex = new UnauthorizedException(""); + ex.putExtraField("isAdmin", (String) null); + Map extraFields = ex.getExtraFields(); + assertThat(extraFields.size()).isEqualTo(1); + assertThat(extraFields.get("isAdmin")).isNull(); + } + + @Test + public void testExtraFields_overrideValue_keepLast() { + UnauthorizedException ex = new UnauthorizedException(""); + ex.putExtraField("isAdmin", "YES"); + ex.putExtraField("isAdmin", TRUE); + Map extraFields = ex.getExtraFields(); + assertThat(extraFields.size()).isEqualTo(1); + assertThat(extraFields.get("isAdmin")).isEqualTo(TRUE); + } + + @Test + public void testExtraFields_ReservedNameDomain_forbidden() { + assertExtraFields_ReservedName_forbidden("domain"); + } + + @Test + public void testExtraFields_ReservedNameMessage_forbidden() { + assertExtraFields_ReservedName_forbidden("message"); + } + + @Test + public void testExtraFields_ReservedNameReason_forbidden() { + assertExtraFields_ReservedName_forbidden("reason"); + } + + private void assertExtraFields_ReservedName_forbidden(String fieldName) { + expectedException.expect(IllegalArgumentException.class); + expectedException.expectMessage("The field name '" + fieldName + "' is reserved"); + + new ConflictException("Fails", "no extra " + fieldName).putExtraField(fieldName, "some other " + fieldName); + } +} diff --git a/endpoints-framework/src/test/java/com/google/api/server/spi/response/RestResponseResultWriterTest.java b/endpoints-framework/src/test/java/com/google/api/server/spi/response/RestResponseResultWriterTest.java index f6cf2b1a..95bc6cc5 100644 --- a/endpoints-framework/src/test/java/com/google/api/server/spi/response/RestResponseResultWriterTest.java +++ b/endpoints-framework/src/test/java/com/google/api/server/spi/response/RestResponseResultWriterTest.java @@ -16,6 +16,8 @@ package com.google.api.server.spi.response; import static com.google.common.truth.Truth.assertThat; +import static java.lang.Boolean.FALSE; +import static java.lang.Boolean.TRUE; import com.google.api.server.spi.ObjectMapperUtil; import com.google.api.server.spi.ServiceException; @@ -27,6 +29,7 @@ import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; +import org.skyscreamer.jsonassert.JSONAssert; import org.springframework.mock.web.MockHttpServletResponse; /** @@ -219,4 +222,51 @@ private void writeError(boolean enableExceptionCompatibility, String customReaso assertThat(innerError.path("domain").asText()).isEqualTo(expectedDomain); assertThat(innerError.path("reason").asText()).isEqualTo(expectedReason); } + + @Test + public void writeError_extraFields() throws Exception { + MockHttpServletResponse response = new MockHttpServletResponse(); + RestResponseResultWriter writer = new RestResponseResultWriter(response, null, true /* prettyPrint */, + true /* addContentLength */, true /* enableExceptionCompatibility */); + + ServiceException serviceException = new ServiceException(400, "customMessage", "customReason", "customDomain"); + // Extra field string + serviceException.putExtraField("someExtraString", "string1") + .putExtraField("someNullString", (String)null); + // Extra field number + serviceException.putExtraField("someExtraInt", Integer.valueOf(12)) + .putExtraField("someExtraFloat", Float.valueOf(1.2f)) + .putExtraField("someNullNumber", (Number)null); + // Extra field boolean + serviceException.putExtraField("someExtraTrue", TRUE) + .putExtraField("someExtraFalse", FALSE) + .putExtraField("someNullBoolean", (Boolean)null); + // Extra field, keys are equals to reserved keywords when ignoring case + serviceException.putExtraField("Domain", TRUE) + .putExtraField("REASON", Long.valueOf(1234567890)) + .putExtraField("messAge", "hello world!"); + + String expectedError = "{\"error\": {\"errors\": [{" + + " \"domain\": \"customDomain\"," + + " \"reason\": \"customReason\"," + + " \"message\": \"customMessage\"," + + " \"someExtraString\": \"string1\"," + + " \"someNullString\": null," + + " \"someExtraInt\": 12," + + " \"someExtraFloat\": 1.2," + + " \"someNullNumber\": null," + + " \"someExtraTrue\": true," + + " \"someExtraFalse\": false," + + " \"someNullBoolean\": null," + + " \"Domain\": true," + + " \"REASON\": \"1234567890\"," + + " \"messAge\": \"hello world!\"" + + " }]," + + " \"code\": 400," + + " \"message\": \"customMessage\"" + + "}}"; + + writer.writeError(serviceException); + JSONAssert.assertEquals(expectedError, response.getContentAsString(), true); + } } diff --git a/gradle.properties b/gradle.properties index 11845479..33b12339 100644 --- a/gradle.properties +++ b/gradle.properties @@ -21,5 +21,6 @@ floggerVersion=0.3.1 junitVersion=4.12 mockitoVersion=1.10.19 +jsonassertVersion=1.5.0 truthVersion=0.28 springtestVersion=3.2.16.RELEASE