JAMES-1676 setMessages: handle parsing error for each update request
Project: http://git-wip-us.apache.org/repos/asf/james-project/repo Commit: http://git-wip-us.apache.org/repos/asf/james-project/commit/41aebb7b Tree: http://git-wip-us.apache.org/repos/asf/james-project/tree/41aebb7b Diff: http://git-wip-us.apache.org/repos/asf/james-project/diff/41aebb7b Branch: refs/heads/master Commit: 41aebb7b01d8feee85e148e90792b966518a4ebe Parents: 958a96e Author: Raphael Ouazana <raphael.ouaz...@linagora.com> Authored: Fri Feb 5 10:40:23 2016 +0100 Committer: Fabien Vignon <fvig...@linagora.com> Committed: Wed Feb 10 17:47:25 2016 +0100 ---------------------------------------------------------------------- .../jmap/methods/SetMessagesMethodTest.java | 6 +-- .../james/jmap/methods/SetMessagesMethod.java | 54 ++++++++++++++++++-- .../james/jmap/model/SetMessagesRequest.java | 11 ++-- 3 files changed, 58 insertions(+), 13 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/james-project/blob/41aebb7b/server/protocols/jmap-integration-testing/src/test/java/org/apache/james/jmap/methods/SetMessagesMethodTest.java ---------------------------------------------------------------------- diff --git a/server/protocols/jmap-integration-testing/src/test/java/org/apache/james/jmap/methods/SetMessagesMethodTest.java b/server/protocols/jmap-integration-testing/src/test/java/org/apache/james/jmap/methods/SetMessagesMethodTest.java index 04921b3..c584f4c 100644 --- a/server/protocols/jmap-integration-testing/src/test/java/org/apache/james/jmap/methods/SetMessagesMethodTest.java +++ b/server/protocols/jmap-integration-testing/src/test/java/org/apache/james/jmap/methods/SetMessagesMethodTest.java @@ -489,7 +489,6 @@ public abstract class SetMessagesMethodTest { } @Test - @Ignore("Unable to deal with invalid types from SetMessages requests handler") public void setMessagesShouldRejectUpdateWhenPropertyHasWrongType() throws MailboxException { jmapServer.serverProbe().createMailbox(MailboxConstants.USER_NAMESPACE, username, "mailbox"); jmapServer.serverProbe().appendMessage(username, new MailboxPath(MailboxConstants.USER_NAMESPACE, username, "mailbox"), @@ -504,7 +503,6 @@ public abstract class SetMessagesMethodTest { .contentType(ContentType.JSON) .header("Authorization", accessToken.serialize()) .body(String.format("[[\"setMessages\", {\"update\": {\"%s\" : { \"isUnread\" : \"123\" } } }, \"#0\"]]", messageId)) - // Does not work, jackson throws InvalidFormatException way before SetMessageMethod can deal with ! .when() .post("/jmap") .then() @@ -513,8 +511,8 @@ public abstract class SetMessagesMethodTest { .body("[0][0]", equalTo("messagesSet")) .body("[0][1].notUpdated", hasKey(messageId)) .body("[0][1].notUpdated[\""+messageId+"\"].type", equalTo("invalidProperties")) - .body("[0][1].notUpdated[\""+messageId+"\"].properties", equalTo("isUnread")) - .body("[0][1].notUpdated[\""+messageId+"\"].description", equalTo("invalid properties")) + .body("[0][1].notUpdated[\""+messageId+"\"].properties[0]", equalTo("isUnread")) + .body("[0][1].notUpdated[\""+messageId+"\"].description", startsWith("Can not construct instance of java.lang.Boolean from String value '123': only \"true\" or \"false\" recognized")) .body("[0][1].updated", hasSize(0)); } http://git-wip-us.apache.org/repos/asf/james-project/blob/41aebb7b/server/protocols/jmap/src/main/java/org/apache/james/jmap/methods/SetMessagesMethod.java ---------------------------------------------------------------------- diff --git a/server/protocols/jmap/src/main/java/org/apache/james/jmap/methods/SetMessagesMethod.java b/server/protocols/jmap/src/main/java/org/apache/james/jmap/methods/SetMessagesMethod.java index c849718..d93b65a 100644 --- a/server/protocols/jmap/src/main/java/org/apache/james/jmap/methods/SetMessagesMethod.java +++ b/server/protocols/jmap/src/main/java/org/apache/james/jmap/methods/SetMessagesMethod.java @@ -19,19 +19,23 @@ package org.apache.james.jmap.methods; +import java.io.IOException; import java.util.Iterator; import java.util.List; import java.util.Map; import java.util.NoSuchElementException; import java.util.function.Consumer; import java.util.stream.Stream; + import javax.inject.Inject; import javax.mail.Flags; import org.apache.james.jmap.exceptions.MessageNotFoundException; +import org.apache.james.jmap.json.ObjectMapperFactory; import org.apache.james.jmap.model.ClientId; import org.apache.james.jmap.model.MessageId; import org.apache.james.jmap.model.MessageProperties; +import org.apache.james.jmap.model.MessageProperties.MessageProperty; import org.apache.james.jmap.model.SetError; import org.apache.james.jmap.model.SetMessagesRequest; import org.apache.james.jmap.model.SetMessagesResponse; @@ -48,8 +52,14 @@ import org.apache.james.mailbox.store.mail.MessageMapper.FetchType; import org.apache.james.mailbox.store.mail.model.Mailbox; import org.apache.james.mailbox.store.mail.model.MailboxId; import org.apache.james.mailbox.store.mail.model.MailboxMessage; +import org.apache.james.util.streams.Collectors; import org.slf4j.Logger; import org.slf4j.LoggerFactory; + +import com.fasterxml.jackson.databind.JsonMappingException; +import com.fasterxml.jackson.databind.JsonMappingException.Reference; +import com.fasterxml.jackson.databind.ObjectMapper; +import com.fasterxml.jackson.databind.node.ObjectNode; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; @@ -65,11 +75,13 @@ public class SetMessagesMethod<Id extends MailboxId> implements Method { private final MailboxMapperFactory<Id> mailboxMapperFactory; private final MailboxSessionMapperFactory<Id> mailboxSessionMapperFactory; + private final ObjectMapper jsonParser; @Inject - @VisibleForTesting SetMessagesMethod(MailboxMapperFactory<Id> mailboxMapperFactory, MailboxSessionMapperFactory<Id> mailboxSessionMapperFactory) { + @VisibleForTesting SetMessagesMethod(MailboxMapperFactory<Id> mailboxMapperFactory, MailboxSessionMapperFactory<Id> mailboxSessionMapperFactory, ObjectMapperFactory objectMapperFactory) { this.mailboxMapperFactory = mailboxMapperFactory; this.mailboxSessionMapperFactory = mailboxSessionMapperFactory; + jsonParser = objectMapperFactory.forParsing(); } @Override @@ -106,11 +118,21 @@ public class SetMessagesMethod<Id extends MailboxId> implements Method { return responseBuilder.build(); } - private void processUpdates(Map<MessageId, UpdateMessagePatch> mapOfMessagePatchesById, MailboxSession mailboxSession, + private void processUpdates(Map<MessageId, ObjectNode> mapOfMessagePatchesById, MailboxSession mailboxSession, SetMessagesResponse.Builder responseBuilder) { mapOfMessagePatchesById.entrySet().stream() - .filter(kv -> kv.getValue().isValid()) - .forEach(kv -> update(kv.getKey(), kv.getValue(), mailboxSession, responseBuilder)); + .forEach(kv -> parseAndUpdate(mailboxSession, responseBuilder, kv.getKey(), kv.getValue())); + } + + private void parseAndUpdate(MailboxSession mailboxSession, SetMessagesResponse.Builder responseBuilder, MessageId messageId, ObjectNode json) { + try { + UpdateMessagePatch updateMessagePatch = jsonParser.readValue(json.toString(), UpdateMessagePatch.class); + update(messageId, updateMessagePatch, mailboxSession, responseBuilder); + } catch (JsonMappingException e) { + handleInvalidField(messageId, responseBuilder, e); + } catch (IOException e) { + handleInvalidRequest(messageId, responseBuilder, e); + } } private void update(MessageId messageId, UpdateMessagePatch updateMessagePatch, MailboxSession mailboxSession, SetMessagesResponse.Builder builder){ @@ -127,6 +149,30 @@ public class SetMessagesMethod<Id extends MailboxId> implements Method { } } + private void handleInvalidField(MessageId messageId, SetMessagesResponse.Builder builder, JsonMappingException e) { + LOGGER.error("Invalid field in update request", e); + builder.notUpdated(ImmutableMap.of(messageId, SetError.builder() + .type("invalidProperties") + .description(e.getMessage()) + .properties(fromJsonReferences(e.getPath())) + .build())); + } + + private ImmutableSet<MessageProperty> fromJsonReferences(List<Reference> references) { + return references.stream() + .map(Reference::getFieldName) + .map(MessageProperty::valueOf) + .collect(Collectors.toImmutableSet()); + } + + private void handleInvalidRequest(MessageId messageId, SetMessagesResponse.Builder builder, IOException e) { + LOGGER.error("Invalid update request", e); + builder.notUpdated(ImmutableMap.of(messageId, SetError.builder() + .type("invalidProperties") + .description(e.getMessage()) + .build())); + } + private void handleMessageUpdateException(MessageId messageId, SetMessagesResponse.Builder builder, MailboxException e) { LOGGER.error("An error occurred when updating a message", e); builder.notUpdated(ImmutableMap.of(messageId, SetError.builder() http://git-wip-us.apache.org/repos/asf/james-project/blob/41aebb7b/server/protocols/jmap/src/main/java/org/apache/james/jmap/model/SetMessagesRequest.java ---------------------------------------------------------------------- diff --git a/server/protocols/jmap/src/main/java/org/apache/james/jmap/model/SetMessagesRequest.java b/server/protocols/jmap/src/main/java/org/apache/james/jmap/model/SetMessagesRequest.java index 1986df0..b1f3a60 100644 --- a/server/protocols/jmap/src/main/java/org/apache/james/jmap/model/SetMessagesRequest.java +++ b/server/protocols/jmap/src/main/java/org/apache/james/jmap/model/SetMessagesRequest.java @@ -28,6 +28,7 @@ import org.apache.james.jmap.methods.JmapRequest; import com.fasterxml.jackson.databind.annotation.JsonDeserialize; import com.fasterxml.jackson.databind.annotation.JsonPOJOBuilder; +import com.fasterxml.jackson.databind.node.ObjectNode; import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; @@ -45,7 +46,7 @@ public class SetMessagesRequest implements JmapRequest { private String accountId; private String ifInState; private ImmutableList.Builder<Message> create; - private ImmutableMap.Builder<MessageId, UpdateMessagePatch> update; + private ImmutableMap.Builder<MessageId, ObjectNode> update; private ImmutableList.Builder<MessageId> destroy; private Builder() { @@ -75,7 +76,7 @@ public class SetMessagesRequest implements JmapRequest { return this; } - public Builder update(Map<MessageId, UpdateMessagePatch> updates) { + public Builder update(Map<MessageId, ObjectNode> updates) { this.update.putAll(updates); return this; } @@ -93,10 +94,10 @@ public class SetMessagesRequest implements JmapRequest { private final Optional<String> accountId; private final Optional<String> ifInState; private final List<Message> create; - private final Map<MessageId, UpdateMessagePatch> update; + private final Map<MessageId, ObjectNode> update; private final List<MessageId> destroy; - @VisibleForTesting SetMessagesRequest(Optional<String> accountId, Optional<String> ifInState, List<Message> create, Map<MessageId, UpdateMessagePatch> update, List<MessageId> destroy) { + @VisibleForTesting SetMessagesRequest(Optional<String> accountId, Optional<String> ifInState, List<Message> create, Map<MessageId, ObjectNode> update, List<MessageId> destroy) { this.accountId = accountId; this.ifInState = ifInState; this.create = create; @@ -116,7 +117,7 @@ public class SetMessagesRequest implements JmapRequest { return create; } - public Map<MessageId, UpdateMessagePatch> getUpdate() { + public Map<MessageId, ObjectNode> getUpdate() { return update; } --------------------------------------------------------------------- To unsubscribe, e-mail: server-dev-unsubscr...@james.apache.org For additional commands, e-mail: server-dev-h...@james.apache.org
