You are viewing a plain text version of this content. The canonical link for it is here.
Posted to server-dev@james.apache.org by ma...@apache.org on 2016/02/16 09:06:19 UTC
james-project git commit: JAMES-1687 Improve error reporting in case
of invalid arguments
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 <ra...@linagora.com>
Authored: Mon Feb 8 17:07:01 2016 +0100
Committer: Matthieu Baechler <ma...@gmail.com>
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: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org