Repository: james-project Updated Branches: refs/heads/master 52316f64b -> 27f1e48d4
JAMES-1888 Cassandra read should not fail on duplicated Attachment representation Simplify code for attachment management Project: http://git-wip-us.apache.org/repos/asf/james-project/repo Commit: http://git-wip-us.apache.org/repos/asf/james-project/commit/bc74873e Tree: http://git-wip-us.apache.org/repos/asf/james-project/tree/bc74873e Diff: http://git-wip-us.apache.org/repos/asf/james-project/diff/bc74873e Branch: refs/heads/master Commit: bc74873e8d4b26f8d6fb39010791c4dbc91649f5 Parents: 9aff85c Author: Benoit Tellier <[email protected]> Authored: Tue Dec 20 12:39:35 2016 +0700 Committer: Benoit Tellier <[email protected]> Committed: Tue Dec 20 15:48:31 2016 +0700 ---------------------------------------------------------------------- .../james/mailbox/model/MessageAttachment.java | 3 +- .../cassandra/mail/AttachmentLoader.java | 14 ++-- .../cassandra/mail/CassandraMessageDAO.java | 2 +- .../mailbox/cassandra/mail/utils/MapMerger.java | 61 -------------- .../cassandra/mail/AttachmentLoaderTest.java | 84 ++++++++++++++++++++ .../cassandra/mail/utils/MapMergerTest.java | 78 ------------------ 6 files changed, 93 insertions(+), 149 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/james-project/blob/bc74873e/mailbox/api/src/main/java/org/apache/james/mailbox/model/MessageAttachment.java ---------------------------------------------------------------------- diff --git a/mailbox/api/src/main/java/org/apache/james/mailbox/model/MessageAttachment.java b/mailbox/api/src/main/java/org/apache/james/mailbox/model/MessageAttachment.java index 1c4c084..f2a68cd 100644 --- a/mailbox/api/src/main/java/org/apache/james/mailbox/model/MessageAttachment.java +++ b/mailbox/api/src/main/java/org/apache/james/mailbox/model/MessageAttachment.java @@ -19,7 +19,6 @@ package org.apache.james.mailbox.model; -import com.google.common.annotations.VisibleForTesting; import com.google.common.base.MoreObjects; import com.google.common.base.Objects; import com.google.common.base.Optional; @@ -88,7 +87,7 @@ public class MessageAttachment { private final Optional<Cid> cid; private final boolean isInline; - @VisibleForTesting MessageAttachment(Attachment attachment, Optional<String> name, Optional<Cid> cid, boolean isInline) { + public MessageAttachment(Attachment attachment, Optional<String> name, Optional<Cid> cid, boolean isInline) { this.attachment = attachment; this.name = name; this.cid = cid; http://git-wip-us.apache.org/repos/asf/james-project/blob/bc74873e/mailbox/cassandra/src/main/java/org/apache/james/mailbox/cassandra/mail/AttachmentLoader.java ---------------------------------------------------------------------- diff --git a/mailbox/cassandra/src/main/java/org/apache/james/mailbox/cassandra/mail/AttachmentLoader.java b/mailbox/cassandra/src/main/java/org/apache/james/mailbox/cassandra/mail/AttachmentLoader.java index c9a81d0..9544c19 100644 --- a/mailbox/cassandra/src/main/java/org/apache/james/mailbox/cassandra/mail/AttachmentLoader.java +++ b/mailbox/cassandra/src/main/java/org/apache/james/mailbox/cassandra/mail/AttachmentLoader.java @@ -22,8 +22,8 @@ import java.util.Collection; import java.util.Set; import java.util.Map; import java.util.function.Function; +import java.util.stream.Collectors; -import org.apache.james.mailbox.cassandra.mail.utils.MapMerger; import org.apache.james.mailbox.model.Attachment; import org.apache.james.mailbox.model.AttachmentId; import org.apache.james.mailbox.model.MessageAttachment; @@ -42,13 +42,13 @@ public class AttachmentLoader { } public Collection<MessageAttachment> getAttachments(Set<CassandraMessageDAO.MessageAttachmentRepresentation> attachmentRepresentations) { + Map<AttachmentId, Attachment> attachmentsById = attachmentsById(attachmentRepresentations.stream() + .map(CassandraMessageDAO.MessageAttachmentRepresentation::getAttachmentId) + .collect(Guavate.toImmutableSet())); - Map<AttachmentId, CassandraMessageDAO.MessageAttachmentRepresentation> attachmentRepresentationsById = attachmentRepresentations.stream() - .collect(Guavate.toImmutableMap(CassandraMessageDAO.MessageAttachmentRepresentation::getAttachmentId, Function.identity())); - - Map<AttachmentId, Attachment> attachmentsById = attachmentsById(attachmentRepresentationsById.keySet()); - - return MapMerger.merge(attachmentsById, attachmentRepresentationsById, this::constructMessageAttachment).values(); + return attachmentRepresentations.stream() + .map(representation -> constructMessageAttachment(attachmentsById.get(representation.getAttachmentId()), representation)) + .collect(Guavate.toImmutableSet()); } private MessageAttachment constructMessageAttachment(Attachment attachment, CassandraMessageDAO.MessageAttachmentRepresentation messageAttachmentRepresentation) { http://git-wip-us.apache.org/repos/asf/james-project/blob/bc74873e/mailbox/cassandra/src/main/java/org/apache/james/mailbox/cassandra/mail/CassandraMessageDAO.java ---------------------------------------------------------------------- diff --git a/mailbox/cassandra/src/main/java/org/apache/james/mailbox/cassandra/mail/CassandraMessageDAO.java b/mailbox/cassandra/src/main/java/org/apache/james/mailbox/cassandra/mail/CassandraMessageDAO.java index dd71f3e..1ac38e5 100644 --- a/mailbox/cassandra/src/main/java/org/apache/james/mailbox/cassandra/mail/CassandraMessageDAO.java +++ b/mailbox/cassandra/src/main/java/org/apache/james/mailbox/cassandra/mail/CassandraMessageDAO.java @@ -321,7 +321,7 @@ public class CassandraMessageDAO { return headerContent; } - static class MessageAttachmentRepresentation { + public static class MessageAttachmentRepresentation { public static Builder builder() { return new Builder(); http://git-wip-us.apache.org/repos/asf/james-project/blob/bc74873e/mailbox/cassandra/src/main/java/org/apache/james/mailbox/cassandra/mail/utils/MapMerger.java ---------------------------------------------------------------------- diff --git a/mailbox/cassandra/src/main/java/org/apache/james/mailbox/cassandra/mail/utils/MapMerger.java b/mailbox/cassandra/src/main/java/org/apache/james/mailbox/cassandra/mail/utils/MapMerger.java deleted file mode 100644 index 2a57224..0000000 --- a/mailbox/cassandra/src/main/java/org/apache/james/mailbox/cassandra/mail/utils/MapMerger.java +++ /dev/null @@ -1,61 +0,0 @@ -/**************************************************************** - * 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.mailbox.cassandra.mail.utils; - -import com.github.steveash.guavate.Guavate; -import com.google.common.annotations.VisibleForTesting; -import org.apache.james.mailbox.cassandra.mail.AttachmentLoader; - -import java.util.Map; -import java.util.function.BiFunction; -import java.util.stream.Stream; - -public class MapMerger { - - public static <K, U, V, T> Map<K, T> merge(Map<K, U> lhs, Map<K, V> rhs, BiFunction<U, V, T> merge) { - return lhs.entrySet().stream() - .flatMap(x -> entry(x.getKey(), x.getValue(), rhs.get(x.getKey()))) - .collect(Guavate.toImmutableMap(Entry::getKey, x -> merge.apply(x.value1, x.value2))); - } - - private static <K, U, V> Stream<Entry<K, U, V>> entry(K k, U u, V v) { - if (v == null) { - return Stream.of(); - } else { - return Stream.of(new Entry(k, u ,v)); - } - } - - private static class Entry<K, U, V> { - private final K key; - private final U value1; - private final V value2; - - Entry(K key, U value1, V value2) { - this.key = key; - this.value1 = value1; - this.value2 = value2; - } - - public K getKey() { - return key; - } - } -} http://git-wip-us.apache.org/repos/asf/james-project/blob/bc74873e/mailbox/cassandra/src/test/java/org/apache/james/mailbox/cassandra/mail/AttachmentLoaderTest.java ---------------------------------------------------------------------- diff --git a/mailbox/cassandra/src/test/java/org/apache/james/mailbox/cassandra/mail/AttachmentLoaderTest.java b/mailbox/cassandra/src/test/java/org/apache/james/mailbox/cassandra/mail/AttachmentLoaderTest.java index a3ba842..72e0607 100644 --- a/mailbox/cassandra/src/test/java/org/apache/james/mailbox/cassandra/mail/AttachmentLoaderTest.java +++ b/mailbox/cassandra/src/test/java/org/apache/james/mailbox/cassandra/mail/AttachmentLoaderTest.java @@ -22,18 +22,24 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; +import java.util.Collection; import java.util.Map; +import java.util.Optional; import java.util.Set; import org.apache.james.mailbox.model.Attachment; import org.apache.james.mailbox.model.AttachmentId; +import org.apache.james.mailbox.model.Cid; +import org.apache.james.mailbox.model.MessageAttachment; import org.apache.james.mailbox.store.mail.AttachmentMapper; +import org.apache.james.util.OptionalConverter; import org.assertj.core.data.MapEntry; import org.junit.Before; import org.junit.Test; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Sets; public class AttachmentLoaderTest { @@ -47,6 +53,84 @@ public class AttachmentLoaderTest { } @Test + public void getAttachmentsShouldWorkWithDuplicatedIds() { + AttachmentId attachmentId = AttachmentId.from("1"); + Set<AttachmentId> attachmentIds = ImmutableSet.of(attachmentId); + + Attachment attachment = Attachment.builder() + .attachmentId(attachmentId) + .bytes("attachment".getBytes()) + .type("type") + .build(); + when(attachmentMapper.getAttachments(attachmentIds)) + .thenReturn(ImmutableList.of(attachment)); + + Optional<String> name1 = Optional.of("name1"); + Optional<String> name2 = Optional.of("name2"); + Optional<Cid> cid = Optional.empty(); + boolean isInlined = false; + CassandraMessageDAO.MessageAttachmentRepresentation attachmentRepresentation1 = new CassandraMessageDAO.MessageAttachmentRepresentation(attachmentId, name1, cid, isInlined); + CassandraMessageDAO.MessageAttachmentRepresentation attachmentRepresentation2 = new CassandraMessageDAO.MessageAttachmentRepresentation(attachmentId, name2, cid, isInlined); + + Collection<MessageAttachment> attachments = testee.getAttachments(Sets.newHashSet(attachmentRepresentation1, attachmentRepresentation2)); + + assertThat(attachments).hasSize(2) + .containsOnly(new MessageAttachment(attachment, OptionalConverter.toGuava(name1), OptionalConverter.toGuava(cid), isInlined), + new MessageAttachment(attachment, OptionalConverter.toGuava(name2), OptionalConverter.toGuava(cid), isInlined)); + } + + @Test + public void getAttachmentsShouldReturnMultipleAttachmentWhenSeveralAttachmentsRepresentation() { + AttachmentId attachmentId1 = AttachmentId.from("1"); + AttachmentId attachmentId2 = AttachmentId.from("2"); + Set<AttachmentId> attachmentIds = ImmutableSet.of(attachmentId1, attachmentId2); + + Attachment attachment1 = Attachment.builder() + .attachmentId(attachmentId1) + .bytes("attachment1".getBytes()) + .type("type") + .build(); + Attachment attachment2 = Attachment.builder() + .attachmentId(attachmentId2) + .bytes("attachment2".getBytes()) + .type("type") + .build(); + when(attachmentMapper.getAttachments(attachmentIds)) + .thenReturn(ImmutableList.of(attachment1, attachment2)); + + Optional<String> name1 = Optional.of("name1"); + Optional<String> name2 = Optional.of("name2"); + Optional<Cid> cid = Optional.empty(); + boolean isInlined = false; + CassandraMessageDAO.MessageAttachmentRepresentation attachmentRepresentation1 = new CassandraMessageDAO.MessageAttachmentRepresentation(attachmentId1, name1, cid, isInlined); + CassandraMessageDAO.MessageAttachmentRepresentation attachmentRepresentation2 = new CassandraMessageDAO.MessageAttachmentRepresentation(attachmentId2, name2, cid, isInlined); + + Collection<MessageAttachment> attachments = testee.getAttachments(Sets.newHashSet(attachmentRepresentation1, attachmentRepresentation2)); + + assertThat(attachments).hasSize(2) + .containsOnly(new MessageAttachment(attachment1, OptionalConverter.toGuava(name1), OptionalConverter.toGuava(cid), isInlined), + new MessageAttachment(attachment2, OptionalConverter.toGuava(name2), OptionalConverter.toGuava(cid), isInlined)); + } + + @Test + public void getAttachmentsShouldReturnEmptyByDefault() { + AttachmentId attachmentId = AttachmentId.from("1"); + Set<AttachmentId> attachmentIds = ImmutableSet.of(attachmentId); + + Attachment attachment = Attachment.builder() + .attachmentId(attachmentId) + .bytes("attachment".getBytes()) + .type("type") + .build(); + when(attachmentMapper.getAttachments(attachmentIds)) + .thenReturn(ImmutableList.of(attachment)); + + Collection<MessageAttachment> attachments = testee.getAttachments(Sets.newHashSet()); + + assertThat(attachments).isEmpty(); + } + + @Test public void attachmentsByIdShouldReturnMapWhenExist() { AttachmentId attachmentId = AttachmentId.from("1"); AttachmentId attachmentId2 = AttachmentId.from("2"); http://git-wip-us.apache.org/repos/asf/james-project/blob/bc74873e/mailbox/cassandra/src/test/java/org/apache/james/mailbox/cassandra/mail/utils/MapMergerTest.java ---------------------------------------------------------------------- diff --git a/mailbox/cassandra/src/test/java/org/apache/james/mailbox/cassandra/mail/utils/MapMergerTest.java b/mailbox/cassandra/src/test/java/org/apache/james/mailbox/cassandra/mail/utils/MapMergerTest.java deleted file mode 100644 index 805ce76..0000000 --- a/mailbox/cassandra/src/test/java/org/apache/james/mailbox/cassandra/mail/utils/MapMergerTest.java +++ /dev/null @@ -1,78 +0,0 @@ -/**************************************************************** - * 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.mailbox.cassandra.mail.utils; - -import static org.assertj.core.api.Assertions.assertThat; - -import java.util.Map; - -import org.junit.Rule; -import org.junit.Test; -import org.junit.rules.ExpectedException; - -import com.google.common.base.Strings; -import com.google.common.collect.ImmutableMap; - -public class MapMergerTest { - - @Rule - public ExpectedException expectedException = ExpectedException.none(); - - @Test - public void mergeShouldReturnMapWhenMapsMatch() { - Map<Integer, String> lhs = ImmutableMap.of(1, "a"); - Map<Integer, Integer> rhs = ImmutableMap.of(1, 2); - Map<Integer, String> actual = MapMerger.merge(lhs, rhs, Strings::repeat); - assertThat(actual).hasSize(1).containsEntry(1, "aa"); - } - - @Test - public void mergeShouldReturnEmptyMapWhenMapsDontMatch() { - Map<Integer, String> lhs = ImmutableMap.of(1, "a"); - Map<Integer, Integer> rhs = ImmutableMap.of(3, 2); - Map<Integer, String> actual = MapMerger.merge(lhs, rhs, Strings::repeat); - assertThat(actual).hasSize(0); - } - - @Test - public void mergeShouldReturnEmptyMapWhenFirstMapIsEmpty() { - Map<Integer, String> lhs = ImmutableMap.of(); - Map<Integer, Integer> rhs = ImmutableMap.of(3, 2); - Map<Integer, String> actual = MapMerger.merge(lhs, rhs, Strings::repeat); - assertThat(actual).hasSize(0); - } - - @Test - public void mergeShouldThrowWhenFirstMapIsNull() { - expectedException.expect(NullPointerException.class); - Map<Integer, String> lhs = null; - Map<Integer, Integer> rhs = ImmutableMap.of(3, 2); - Map<Integer, String> actual = MapMerger.merge(lhs, rhs, Strings::repeat); - assertThat(actual).hasSize(0); - } - - @Test - public void mergeThrowWhenSecondMapIsNull() { - Map<Integer, String> lhs = ImmutableMap.of(1, "b"); - Map<Integer, Integer> rhs = null; - expectedException.expect(NullPointerException.class); - MapMerger.merge(lhs, rhs, Strings::repeat); - } -} --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
