Repository: james-project Updated Branches: refs/heads/master d3b49527c -> e4424ad39
JAMES-1687 Improve error reporting in case of invalid arguments Project: http://git-wip-us.apache.org/repos/asf/james-project/repo Commit: http://git-wip-us.apache.org/repos/asf/james-project/commit/e4424ad3 Tree: http://git-wip-us.apache.org/repos/asf/james-project/tree/e4424ad3 Diff: http://git-wip-us.apache.org/repos/asf/james-project/diff/e4424ad3 Branch: refs/heads/master Commit: e4424ad39259b5d835363d7e04b90f5662207dcc Parents: d3b4952 Author: Raphael Ouazana <[email protected]> Authored: Mon Feb 8 17:07:01 2016 +0100 Committer: Matthieu Baechler <[email protected]> Committed: Tue Feb 16 09:05:49 2016 +0100 ---------------------------------------------------------------------- .../jmap/methods/GetMailboxesMethodTest.java | 4 +- .../jmap/methods/GetMessageListMethodTest.java | 4 +- .../jmap/methods/GetMessagesMethodTest.java | 3 +- .../james/jmap/methods/ErrorResponse.java | 72 ++++++++++++++++ .../apache/james/jmap/methods/JmapResponse.java | 32 +++---- .../james/jmap/methods/RequestHandler.java | 25 ++++-- .../org/apache/james/jmap/JMAPServletTest.java | 8 +- .../james/jmap/methods/ErrorResponseTest.java | 89 ++++++++++++++++++++ .../methods/JmapResponseWriterImplTest.java | 30 ++++++- 9 files changed, 233 insertions(+), 34 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/james-project/blob/e4424ad3/server/protocols/jmap-integration-testing/src/test/java/org/apache/james/jmap/methods/GetMailboxesMethodTest.java ---------------------------------------------------------------------- diff --git a/server/protocols/jmap-integration-testing/src/test/java/org/apache/james/jmap/methods/GetMailboxesMethodTest.java b/server/protocols/jmap-integration-testing/src/test/java/org/apache/james/jmap/methods/GetMailboxesMethodTest.java index 0d206d0..047e1d9 100644 --- a/server/protocols/jmap-integration-testing/src/test/java/org/apache/james/jmap/methods/GetMailboxesMethodTest.java +++ b/server/protocols/jmap-integration-testing/src/test/java/org/apache/james/jmap/methods/GetMailboxesMethodTest.java @@ -222,7 +222,9 @@ public abstract class GetMailboxesMethodTest { .then() .statusCode(200) .body(NAME, equalTo("error")) - .body(ARGUMENTS + ".type", equalTo("invalidArguments")); + .body(ARGUMENTS + ".type", equalTo("invalidArguments")) + .body(ARGUMENTS + ".description", equalTo("Can not deserialize instance of java.util.ArrayList out of VALUE_TRUE token\n" + + " at [Source: {\"ids\":true}; line: 1, column: 2] (through reference chain: org.apache.james.jmap.model.Builder[\"ids\"])")); } @Test http://git-wip-us.apache.org/repos/asf/james-project/blob/e4424ad3/server/protocols/jmap-integration-testing/src/test/java/org/apache/james/jmap/methods/GetMessageListMethodTest.java ---------------------------------------------------------------------- diff --git a/server/protocols/jmap-integration-testing/src/test/java/org/apache/james/jmap/methods/GetMessageListMethodTest.java b/server/protocols/jmap-integration-testing/src/test/java/org/apache/james/jmap/methods/GetMessageListMethodTest.java index ca2460d..294e394 100644 --- a/server/protocols/jmap-integration-testing/src/test/java/org/apache/james/jmap/methods/GetMessageListMethodTest.java +++ b/server/protocols/jmap-integration-testing/src/test/java/org/apache/james/jmap/methods/GetMessageListMethodTest.java @@ -98,7 +98,9 @@ public abstract class GetMessageListMethodTest { .then() .statusCode(200) .body(NAME, equalTo("error")) - .body(ARGUMENTS + ".type", equalTo("invalidArguments")); + .body(ARGUMENTS + ".type", equalTo("invalidArguments")) + .body(ARGUMENTS + ".description", equalTo("Can not instantiate value of type [simple type, class org.apache.james.jmap.model.FilterCondition$Builder] from Boolean value (true); no single-boolean/Boolean-arg constructor/factory method\n" + + " at [Source: {\"filter\":true}; line: 1, column: 2] (through reference chain: org.apache.james.jmap.model.Builder[\"filter\"])")); } @Test http://git-wip-us.apache.org/repos/asf/james-project/blob/e4424ad3/server/protocols/jmap-integration-testing/src/test/java/org/apache/james/jmap/methods/GetMessagesMethodTest.java ---------------------------------------------------------------------- diff --git a/server/protocols/jmap-integration-testing/src/test/java/org/apache/james/jmap/methods/GetMessagesMethodTest.java b/server/protocols/jmap-integration-testing/src/test/java/org/apache/james/jmap/methods/GetMessagesMethodTest.java index 26fcb1e..1517ae0 100644 --- a/server/protocols/jmap-integration-testing/src/test/java/org/apache/james/jmap/methods/GetMessagesMethodTest.java +++ b/server/protocols/jmap-integration-testing/src/test/java/org/apache/james/jmap/methods/GetMessagesMethodTest.java @@ -128,7 +128,8 @@ public abstract class GetMessagesMethodTest { .then() .statusCode(200) .body(NAME, equalTo("error")) - .body(ARGUMENTS + ".type", equalTo("invalidArguments")); + .body(ARGUMENTS + ".type", equalTo("invalidArguments")) + .body(ARGUMENTS + ".description", equalTo("N/A (through reference chain: org.apache.james.jmap.model.Builder[\"ids\"])")); } @Test http://git-wip-us.apache.org/repos/asf/james-project/blob/e4424ad3/server/protocols/jmap/src/main/java/org/apache/james/jmap/methods/ErrorResponse.java ---------------------------------------------------------------------- diff --git a/server/protocols/jmap/src/main/java/org/apache/james/jmap/methods/ErrorResponse.java b/server/protocols/jmap/src/main/java/org/apache/james/jmap/methods/ErrorResponse.java new file mode 100644 index 0000000..32b2c36 --- /dev/null +++ b/server/protocols/jmap/src/main/java/org/apache/james/jmap/methods/ErrorResponse.java @@ -0,0 +1,72 @@ +/**************************************************************** + * Licensed to the Apache Software Foundation (ASF) under one * + * or more contributor license agreements. See the NOTICE file * + * distributed with this work for additional information * + * regarding copyright ownership. The ASF licenses this file * + * to you under the Apache License, Version 2.0 (the * + * "License"); you may not use this file except in compliance * + * with the License. You may obtain a copy of the License at * + * * + * http://www.apache.org/licenses/LICENSE-2.0 * + * * + * Unless required by applicable law or agreed to in writing, * + * software distributed under the License is distributed on an * + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY * + * KIND, either express or implied. See the License for the * + * specific language governing permissions and limitations * + * under the License. * + ****************************************************************/ + +package org.apache.james.jmap.methods; + +import java.util.Optional; + +import com.google.common.annotations.VisibleForTesting; + +public class ErrorResponse implements Method.Response { + public static final Method.Response.Name ERROR_METHOD = Method.Response.name("error"); + @VisibleForTesting static final String DEFAULT_ERROR_MESSAGE = "Error while processing"; + + public static Builder builder() { + return new Builder(); + } + + public static class Builder { + + private Optional<String> type = Optional.empty(); + private Optional<String> description = Optional.empty(); + + private Builder() { + } + + public Builder type(String type) { + this.type = Optional.ofNullable(type); + return this; + } + + public Builder description(String description) { + this.description = Optional.ofNullable(description); + return this; + } + + public ErrorResponse build() { + return new ErrorResponse(type.orElse(DEFAULT_ERROR_MESSAGE), description); + } + } + + private final String type; + private final Optional<String> description; + + @VisibleForTesting ErrorResponse(String type, Optional<String> description) { + this.type = type; + this.description = description; + } + + public String getType() { + return type; + } + + public Optional<String> getDescription() { + return description; + } +} http://git-wip-us.apache.org/repos/asf/james-project/blob/e4424ad3/server/protocols/jmap/src/main/java/org/apache/james/jmap/methods/JmapResponse.java ---------------------------------------------------------------------- diff --git a/server/protocols/jmap/src/main/java/org/apache/james/jmap/methods/JmapResponse.java b/server/protocols/jmap/src/main/java/org/apache/james/jmap/methods/JmapResponse.java index 0af70e8..db15434 100644 --- a/server/protocols/jmap/src/main/java/org/apache/james/jmap/methods/JmapResponse.java +++ b/server/protocols/jmap/src/main/java/org/apache/james/jmap/methods/JmapResponse.java @@ -26,7 +26,6 @@ import org.apache.james.jmap.model.ClientId; import org.apache.james.jmap.model.Property; import com.fasterxml.jackson.databind.ser.impl.SimpleFilterProvider; -import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ImmutableSet; public class JmapResponse { @@ -76,38 +75,29 @@ public class JmapResponse { } public Builder error() { - return error(DEFAULT_ERROR_MESSAGE); + this.response = ErrorResponse.builder().build(); + this.responseName = ErrorResponse.ERROR_METHOD; + return this; } public Builder error(String message) { - this.response = new ErrorResponse(message); - this.responseName = ERROR_METHOD; + this.response = ErrorResponse.builder().type(message).build(); + this.responseName = ErrorResponse.ERROR_METHOD; return this; } - - public JmapResponse build() { - return new JmapResponse(responseName, id, response, properties, filterProvider); + public Builder error(ErrorResponse error) { + this.response = error; + this.responseName = ErrorResponse.ERROR_METHOD; + return this; } - } - public static class ErrorResponse implements Method.Response { - private final String type; - - public ErrorResponse(String type) { - this.type = type; - } - - public String getType() { - return type; + public JmapResponse build() { + return new JmapResponse(responseName, id, response, properties, filterProvider); } } - - @VisibleForTesting static final String DEFAULT_ERROR_MESSAGE = "Error while processing"; - public static final Method.Response.Name ERROR_METHOD = Method.Response.name("error"); - private final Method.Response.Name method; private final ClientId clientId; private final Method.Response response; http://git-wip-us.apache.org/repos/asf/james-project/blob/e4424ad3/server/protocols/jmap/src/main/java/org/apache/james/jmap/methods/RequestHandler.java ---------------------------------------------------------------------- diff --git a/server/protocols/jmap/src/main/java/org/apache/james/jmap/methods/RequestHandler.java b/server/protocols/jmap/src/main/java/org/apache/james/jmap/methods/RequestHandler.java index 0294aa6..5dd75de 100644 --- a/server/protocols/jmap/src/main/java/org/apache/james/jmap/methods/RequestHandler.java +++ b/server/protocols/jmap/src/main/java/org/apache/james/jmap/methods/RequestHandler.java @@ -68,16 +68,31 @@ public class RequestHandler { } catch (IOException e) { LOGGER.error("Error occured while parsing the request.", e); if (e.getCause() instanceof NotImplementedException) { - return error(request, "Not yet implemented"); + return errorNotImplemented(request); } - return error(request, "invalidArguments"); + return error(request, ErrorResponse.builder() + .type("invalidArguments") + .description(e.getMessage()) + .build()); } catch (NotImplementedException e) { - return error(request, "Not yet implemented"); + return errorNotImplemented(request); } }; } - private Stream<JmapResponse> error(AuthenticatedProtocolRequest request, String message) { - return Stream.of(JmapResponse.builder().clientId(request.getClientId()).error(message).build()); + private Stream<JmapResponse> errorNotImplemented(AuthenticatedProtocolRequest request) { + return Stream.of( + JmapResponse.builder() + .clientId(request.getClientId()) + .error("Not yet implemented") + .build()); + } + + private Stream<JmapResponse> error(AuthenticatedProtocolRequest request, ErrorResponse error) { + return Stream.of( + JmapResponse.builder() + .clientId(request.getClientId()) + .error(error) + .build()); } } http://git-wip-us.apache.org/repos/asf/james-project/blob/e4424ad3/server/protocols/jmap/src/test/java/org/apache/james/jmap/JMAPServletTest.java ---------------------------------------------------------------------- diff --git a/server/protocols/jmap/src/test/java/org/apache/james/jmap/JMAPServletTest.java b/server/protocols/jmap/src/test/java/org/apache/james/jmap/JMAPServletTest.java index f4971d4..b09d9f1 100644 --- a/server/protocols/jmap/src/test/java/org/apache/james/jmap/JMAPServletTest.java +++ b/server/protocols/jmap/src/test/java/org/apache/james/jmap/JMAPServletTest.java @@ -26,9 +26,11 @@ import static org.mockito.Matchers.any; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; +import java.util.stream.Stream; + import org.apache.james.http.jetty.Configuration; import org.apache.james.http.jetty.JettyHttpServer; -import org.apache.james.jmap.methods.JmapResponse; +import org.apache.james.jmap.methods.ErrorResponse; import org.apache.james.jmap.methods.Method; import org.apache.james.jmap.methods.RequestHandler; import org.apache.james.jmap.model.ClientId; @@ -44,8 +46,6 @@ import com.google.common.base.Charsets; import com.jayway.restassured.RestAssured; import com.jayway.restassured.http.ContentType; -import java.util.stream.Stream; - public class JMAPServletTest { private JettyHttpServer server; @@ -94,7 +94,7 @@ public class JMAPServletTest { json.put("type", "invalidArgument"); when(requestHandler.handle(any())) - .thenReturn(Stream.of(new ProtocolResponse(JmapResponse.ERROR_METHOD, json, ClientId.of("#0")))); + .thenReturn(Stream.of(new ProtocolResponse(ErrorResponse.ERROR_METHOD, json, ClientId.of("#0")))); given() .accept(ContentType.JSON) http://git-wip-us.apache.org/repos/asf/james-project/blob/e4424ad3/server/protocols/jmap/src/test/java/org/apache/james/jmap/methods/ErrorResponseTest.java ---------------------------------------------------------------------- diff --git a/server/protocols/jmap/src/test/java/org/apache/james/jmap/methods/ErrorResponseTest.java b/server/protocols/jmap/src/test/java/org/apache/james/jmap/methods/ErrorResponseTest.java new file mode 100644 index 0000000..b1041f9 --- /dev/null +++ b/server/protocols/jmap/src/test/java/org/apache/james/jmap/methods/ErrorResponseTest.java @@ -0,0 +1,89 @@ +/**************************************************************** + * Licensed to the Apache Software Foundation (ASF) under one * + * or more contributor license agreements. See the NOTICE file * + * distributed with this work for additional information * + * regarding copyright ownership. The ASF licenses this file * + * to you under the Apache License, Version 2.0 (the * + * "License"); you may not use this file except in compliance * + * with the License. You may obtain a copy of the License at * + * * + * http://www.apache.org/licenses/LICENSE-2.0 * + * * + * Unless required by applicable law or agreed to in writing, * + * software distributed under the License is distributed on an * + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY * + * KIND, either express or implied. See the License for the * + * specific language governing permissions and limitations * + * under the License. * + ****************************************************************/ +package org.apache.james.jmap.methods; + +import static org.assertj.core.api.Assertions.assertThat; + +import java.util.Optional; + +import org.junit.Test; + +public class ErrorResponseTest { + + @Test + public void buildShouldReturnDefaultErrorWhenNoParameter() { + ErrorResponse expected = new ErrorResponse(ErrorResponse.DEFAULT_ERROR_MESSAGE, Optional.empty()); + + ErrorResponse actual = ErrorResponse + .builder() + .build(); + + assertThat(actual).isEqualToComparingFieldByField(expected); + } + + @Test + public void buildShouldReturnDefaultErrorWhenNullParameter() { + ErrorResponse expected = new ErrorResponse(ErrorResponse.DEFAULT_ERROR_MESSAGE, Optional.empty()); + + ErrorResponse actual = ErrorResponse + .builder() + .type(null) + .build(); + + assertThat(actual).isEqualToComparingFieldByField(expected); + } + + @Test + public void buildShouldReturnDefinedErrorWhenTypeParameter() { + ErrorResponse expected = new ErrorResponse("my error", Optional.empty()); + + ErrorResponse actual = ErrorResponse + .builder() + .type("my error") + .build(); + + assertThat(actual).isEqualToComparingFieldByField(expected); + } + + @Test + public void buildShouldReturnDefinedErrorWhenTypeParameterAndNullDescription() { + ErrorResponse expected = new ErrorResponse("my error", Optional.empty()); + + ErrorResponse actual = ErrorResponse + .builder() + .type("my error") + .description(null) + .build(); + + assertThat(actual).isEqualToComparingFieldByField(expected); + } + + @Test + public void buildShouldReturnDefinedErrorWithDescriptionWhenTypeAndDescriptionParameters() { + ErrorResponse expected = new ErrorResponse("my error", Optional.of("custom description")); + + ErrorResponse actual = ErrorResponse + .builder() + .type("my error") + .description("custom description") + .build(); + + assertThat(actual).isEqualToComparingFieldByField(expected); + } +} http://git-wip-us.apache.org/repos/asf/james-project/blob/e4424ad3/server/protocols/jmap/src/test/java/org/apache/james/jmap/methods/JmapResponseWriterImplTest.java ---------------------------------------------------------------------- diff --git a/server/protocols/jmap/src/test/java/org/apache/james/jmap/methods/JmapResponseWriterImplTest.java b/server/protocols/jmap/src/test/java/org/apache/james/jmap/methods/JmapResponseWriterImplTest.java index 22fe8a3..8f35e43 100644 --- a/server/protocols/jmap/src/test/java/org/apache/james/jmap/methods/JmapResponseWriterImplTest.java +++ b/server/protocols/jmap/src/test/java/org/apache/james/jmap/methods/JmapResponseWriterImplTest.java @@ -221,7 +221,35 @@ public class JmapResponseWriterImplTest { assertThat(response).hasSize(1) .extracting(ProtocolResponse::getResponseName, x -> x.getResults().get("type").asText(), ProtocolResponse::getClientId) - .containsExactly(tuple(JmapResponse.ERROR_METHOD, JmapResponse.DEFAULT_ERROR_MESSAGE, ClientId.of(expectedClientId))); + .containsExactly(tuple(ErrorResponse.ERROR_METHOD, ErrorResponse.DEFAULT_ERROR_MESSAGE, ClientId.of(expectedClientId))); + } + + @Test + public void formatErrorResponseShouldWorkWithTypeAndDescription() { + String expectedClientId = "#1"; + + ObjectNode parameters = new ObjectNode(new JsonNodeFactory(false)); + parameters.put("id", "myId"); + JsonNode[] nodes = new JsonNode[] { new ObjectNode(new JsonNodeFactory(false)).textNode("unknwonMethod"), + parameters, + new ObjectNode(new JsonNodeFactory(false)).textNode(expectedClientId)} ; + + JmapResponseWriterImpl jmapResponseWriterImpl = new JmapResponseWriterImpl(new ObjectMapperFactory()); + List<ProtocolResponse> response = jmapResponseWriterImpl.formatMethodResponse( + Stream.of(JmapResponse + .builder() + .clientId(ProtocolRequest.deserialize(nodes).getClientId()) + .error(ErrorResponse + .builder() + .type("errorType") + .description("complete description") + .build()) + .build())) + .collect(Collectors.toList()); + + assertThat(response).hasSize(1) + .extracting(ProtocolResponse::getResponseName, x -> x.getResults().get("type").asText(), x -> x.getResults().get("description").asText(), ProtocolResponse::getClientId) + .containsExactly(tuple(ErrorResponse.ERROR_METHOD, "errorType", "complete description", ClientId.of(expectedClientId))); } } --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
