JAMES-2048 We should not reject blank CID already stored
Project: http://git-wip-us.apache.org/repos/asf/james-project/repo Commit: http://git-wip-us.apache.org/repos/asf/james-project/commit/eaf503e6 Tree: http://git-wip-us.apache.org/repos/asf/james-project/tree/eaf503e6 Diff: http://git-wip-us.apache.org/repos/asf/james-project/diff/eaf503e6 Branch: refs/heads/master Commit: eaf503e66fba243bf368df474a3276ebb656c216 Parents: 1046e4d Author: benwa <btell...@linagora.com> Authored: Thu Aug 3 18:09:20 2017 +0700 Committer: benwa <btell...@linagora.com> Committed: Tue Aug 8 17:11:05 2017 +0700 ---------------------------------------------------------------------- .../org/apache/james/mailbox/model/Cid.java | 118 +++++++-- .../org/apache/james/mailbox/model/CidTest.java | 250 ++++++++++++++++++- .../cassandra/mail/CassandraMessageDAO.java | 6 +- .../cassandra/mail/CassandraMessageDAOV2.java | 7 +- .../store/mail/model/impl/MessageParser.java | 43 ++-- 5 files changed, 380 insertions(+), 44 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/james-project/blob/eaf503e6/mailbox/api/src/main/java/org/apache/james/mailbox/model/Cid.java ---------------------------------------------------------------------- diff --git a/mailbox/api/src/main/java/org/apache/james/mailbox/model/Cid.java b/mailbox/api/src/main/java/org/apache/james/mailbox/model/Cid.java index 96440e5..612ee32 100644 --- a/mailbox/api/src/main/java/org/apache/james/mailbox/model/Cid.java +++ b/mailbox/api/src/main/java/org/apache/james/mailbox/model/Cid.java @@ -23,32 +23,120 @@ package org.apache.james.mailbox.model; import org.apache.commons.lang.StringUtils; import com.google.common.base.Objects; +import com.google.common.base.Optional; import com.google.common.base.Preconditions; +import com.google.common.base.Strings; public class Cid { - public static Cid from(String cidAsString) { - Preconditions.checkArgument(!StringUtils.isBlank(cidAsString), "'cidAsString' is mandatory"); - return new Cid(normalizedCid(cidAsString)); + public static final StrictCidValidator DEFAULT_VALIDATOR = new StrictCidValidator(); + + public interface CidTransformation { + Optional<Cid> apply(CidValidator cidValidator, String value); } - private static String normalizedCid(String input) { - if (isWrappedWithAngleBrackets(input)) { - return unwrap(input); + public static class Identity implements CidTransformation { + @Override + public Optional<Cid> apply(CidValidator cidValidator, String value) { + return toCid(cidValidator, value); } - return input; } - - private static String unwrap(String cidAsString) { - String unwrapCid = cidAsString.substring(1, cidAsString.length() - 1); - if (StringUtils.isBlank(unwrapCid)) { - throw new IllegalArgumentException("'cidAsString' is mandatory"); + + public static class Unwrap implements CidTransformation { + @Override + public Optional<Cid> apply(CidValidator cidValidator, String value) { + cidValidator.validate(value); + if (isWrappedWithAngleBrackets(value)) { + return unwrap(value, cidValidator); + } + return toCid(cidValidator, value); + } + + + private Optional<Cid> unwrap(String cidAsString, CidValidator cidValidator) { + String unwrapCid = cidAsString.substring(1, cidAsString.length() - 1); + return toCid(cidValidator, unwrapCid); + } + + private boolean isWrappedWithAngleBrackets(String cidAsString) { + return cidAsString != null + && cidAsString.startsWith("<") + && cidAsString.endsWith(">"); + } + } + + public interface CidValidator { + void validate(String cidValue); + } + + public static class StrictCidValidator implements CidValidator { + @Override + public void validate(String cidValue) { + Preconditions.checkArgument(!Strings.isNullOrEmpty(cidValue)); + Preconditions.checkArgument(!StringUtils.isBlank(cidValue), "'cidAsString' is mandatory"); + } + } + + public static class RelaxedCidValidator implements CidValidator { + @Override + public void validate(String cidValue) { + + } + } + + public static class CidParser { + private Optional<CidValidator> validator; + private Optional<CidTransformation> transformation; + + private CidParser() { + validator = Optional.absent(); + transformation = Optional.absent(); + } + + public CidParser relaxed() { + validator = Optional.<CidValidator>of(new RelaxedCidValidator()); + return this; + } + + public CidParser strict() { + validator = Optional.<CidValidator>of(new StrictCidValidator()); + return this; + } + + public CidParser unwrap() { + transformation = Optional.<CidTransformation>of(new Unwrap()); + return this; + } + + public Optional<Cid> parse(String value) { + CidValidator cidValidator = validator.or(DEFAULT_VALIDATOR); + CidTransformation cidTransformation = transformation.or(new Identity()); + return cidTransformation.apply(cidValidator, value); } - return unwrapCid; } - private static boolean isWrappedWithAngleBrackets(String cidAsString) { - return cidAsString.startsWith("<") && cidAsString.endsWith(">"); + public static CidParser parser() { + return new CidParser(); + } + + public static Cid from(String value) { + return parser() + .strict() + .unwrap() + .parse(value) + .get(); + } + + private static Optional<Cid> toCid(CidValidator cidValidator, String value) { + cidValidator.validate(value); + return toCid(value); + } + + private static Optional<Cid> toCid(String cidAsString) { + if (Strings.isNullOrEmpty(cidAsString) || StringUtils.isBlank(cidAsString)) { + return Optional.absent(); + } + return Optional.of(new Cid(cidAsString)); } private final String value; http://git-wip-us.apache.org/repos/asf/james-project/blob/eaf503e6/mailbox/api/src/test/java/org/apache/james/mailbox/model/CidTest.java ---------------------------------------------------------------------- diff --git a/mailbox/api/src/test/java/org/apache/james/mailbox/model/CidTest.java b/mailbox/api/src/test/java/org/apache/james/mailbox/model/CidTest.java index 20163ad..ef05548 100644 --- a/mailbox/api/src/test/java/org/apache/james/mailbox/model/CidTest.java +++ b/mailbox/api/src/test/java/org/apache/james/mailbox/model/CidTest.java @@ -20,24 +20,26 @@ package org.apache.james.mailbox.model; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.guava.api.Assertions.assertThat; -import org.apache.james.mailbox.model.Cid; import org.junit.Rule; import org.junit.Test; import org.junit.rules.ExpectedException; +import com.google.common.base.Optional; + import nl.jqno.equalsverifier.EqualsVerifier; public class CidTest { @Rule public ExpectedException expectedException = ExpectedException.none(); - + @Test public void fromShouldThrowWhenNull() { expectedException.expect(IllegalArgumentException.class); Cid.from(null); } - + @Test public void fromShouldThrowWhenEmpty() { expectedException.expect(IllegalArgumentException.class); @@ -67,19 +69,19 @@ public class CidTest { Cid cid = Cid.from("<123>"); assertThat(cid.getValue()).isEqualTo("123"); } - + @Test public void fromShouldNotRemoveTagsWhenNone() { Cid cid = Cid.from("123"); assertThat(cid.getValue()).isEqualTo("123"); } - + @Test public void fromShouldNotRemoveTagsWhenNotEndTag() { Cid cid = Cid.from("<123"); assertThat(cid.getValue()).isEqualTo("<123"); } - + @Test public void fromShouldNotRemoveTagsWhenNotStartTag() { Cid cid = Cid.from("123>"); @@ -87,6 +89,242 @@ public class CidTest { } @Test + public void fromRelaxedNoUnwrapShouldReturnAbsentWhenNull() { + assertThat(Cid.parser() + .relaxed() + .parse(null)) + .isAbsent(); + } + + @Test + public void fromRelaxedNoUnwrapShouldReturnAbsentWhenEmpty() { + assertThat(Cid.parser() + .relaxed() + .parse("")) + .isAbsent(); + } + + @Test + public void fromRelaxedNoUnwrapShouldReturnAbsentWhenBlank() { + assertThat(Cid.parser() + .relaxed() + .parse(" ")) + .isAbsent(); + } + + @Test + public void fromRelaxedNoUnwrapShouldReturnCidWhenEmptyAfterRemoveTags() { + Optional<Cid> actual = Cid.parser() + .relaxed() + .parse("<>"); + assertThat(actual).isPresent(); + assertThat(actual.get().getValue()).isEqualTo("<>"); + } + + @Test + public void fromRelaxedNoUnwrapShouldReturnCidWhenBlankAfterRemoveTags() { + Optional<Cid> actual = Cid.parser() + .relaxed() + .parse("< >"); + assertThat(actual).isPresent(); + assertThat(actual.get().getValue()).isEqualTo("< >"); + } + + @Test + public void fromRelaxedNoUnwrapShouldNotRemoveTagsWhenExists() { + Optional<Cid> actual = Cid.parser() + .relaxed() + .parse("<123>"); + assertThat(actual).isPresent(); + assertThat(actual.get().getValue()).isEqualTo("<123>"); + } + + @Test + public void fromRelaxedNoUnwrapShouldNotRemoveTagsWhenNone() { + assertThat(Cid.parser() + .relaxed() + .parse("123")) + .contains(Cid.from("123")); + } + + @Test + public void fromRelaxedNoUnwrapShouldNotRemoveTagsWhenNotEndTag() { + assertThat(Cid.parser() + .relaxed() + .parse("<123")) + .contains(Cid.from("<123")); + } + + @Test + public void fromRelaxedNoUnwrapShouldNotRemoveTagsWhenNotStartTag() { + assertThat(Cid.parser() + .relaxed() + .parse("123>")) + .contains(Cid.from("123>")); + } + + + @Test + public void fromRelaxedUnwrapShouldReturnAbsentWhenNull() { + assertThat(Cid.parser() + .relaxed() + .unwrap() + .parse(null)) + .isAbsent(); + } + + @Test + public void fromRelaxedUnwrapShouldReturnAbsentWhenEmpty() { + assertThat(Cid.parser() + .relaxed() + .unwrap() + .parse("")) + .isAbsent(); + } + + @Test + public void fromRelaxedUnwrapShouldReturnAbsentWhenBlank() { + assertThat(Cid.parser() + .relaxed() + .unwrap() + .parse(" ")) + .isAbsent(); + } + + @Test + public void fromRelaxedUnwrapShouldReturnAbsentWhenEmptyAfterRemoveTags() { + assertThat(Cid.parser() + .relaxed() + .unwrap() + .parse("<>")) + .isAbsent(); + } + + @Test + public void fromRelaxedUnwrapShouldReturnAbsentWhenBlankAfterRemoveTags() { + assertThat(Cid.parser() + .relaxed() + .unwrap() + .parse("< >")) + .isAbsent(); + } + + @Test + public void fromRelaxedUnwrapShouldRemoveTagsWhenExists() { + Optional<Cid> actual = Cid.parser() + .relaxed() + .unwrap() + .parse("<123>"); + assertThat(actual).isPresent(); + assertThat(actual.get().getValue()).isEqualTo("123"); + } + + @Test + public void fromRelaxedUnwrapShouldNotRemoveTagsWhenNone() { + assertThat(Cid.parser() + .relaxed() + .unwrap() + .parse("123")) + .contains(Cid.from("123")); + } + + @Test + public void fromRelaxedUnwrapShouldNotRemoveTagsWhenNotEndTag() { + assertThat(Cid.parser() + .relaxed() + .unwrap() + .parse("<123")) + .contains(Cid.from("<123")); + } + + @Test + public void fromRelaxedUnwrapShouldNotRemoveTagsWhenNotStartTag() { + assertThat(Cid.parser() + .relaxed() + .unwrap() + .parse("123>")) + .contains(Cid.from("123>")); + } + + @Test + public void fromStrictNoUnwrapShouldThrowWhenNull() { + expectedException.expect(IllegalArgumentException.class); + + Cid.parser() + .strict() + .parse(null); + } + + @Test + public void fromStrictNoUnwrapShouldThrowWhenEmpty() { + expectedException.expect(IllegalArgumentException.class); + + Cid.parser() + .strict() + .parse(""); + } + + @Test + public void fromStrinctNoUnwrapShouldThrowWhenBlank() { + expectedException.expect(IllegalArgumentException.class); + + Cid.parser() + .strict() + .parse(" "); + } + + @Test + public void fromStrictNoUnwrapShouldNotRemoveTagWhenEmptyAfterRemoveTags() { + Optional<Cid> actual = Cid.parser() + .strict() + .parse("<>"); + assertThat(actual).isPresent(); + assertThat(actual.get().getValue()).isEqualTo("<>"); + } + + @Test + public void fromStrictNoUnwrapShouldNotRemoveTagWhenBlankAfterRemoveTags() { + Optional<Cid> actual = Cid.parser() + .strict() + .parse("< >"); + assertThat(actual).isPresent(); + assertThat(actual.get().getValue()).isEqualTo("< >"); + } + + @Test + public void fromStrictNoUnwrapShouldNotRemoveTagsWhenExists() { + Optional<Cid> actual = Cid.parser() + .strict() + .parse("<123>"); + assertThat(actual).isPresent(); + assertThat(actual.get().getValue()).isEqualTo("<123>"); + } + + @Test + public void fromStrictNoUnwrapShouldNotRemoveTagsWhenNone() { + assertThat(Cid.parser() + .strict() + .parse("123")) + .contains(Cid.from("123")); + } + + @Test + public void fromStrictNoUnwrapShouldNotRemoveTagsWhenNotEndTag() { + assertThat(Cid.parser() + .strict() + .parse("<123")) + .contains(Cid.from("<123")); + } + + @Test + public void fromStrictNoUnwrapShouldNotRemoveTagsWhenNotStartTag() { + assertThat(Cid.parser() + .strict() + .parse("123>")) + .contains(Cid.from("123>")); + } + + @Test public void shouldRespectJavaBeanContract() { EqualsVerifier.forClass(Cid.class).verify(); } http://git-wip-us.apache.org/repos/asf/james-project/blob/eaf503e6/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 1c73063..35c6a3f 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 @@ -74,6 +74,7 @@ import org.apache.james.mailbox.store.mail.model.impl.PropertyBuilder; import org.apache.james.mailbox.store.mail.model.impl.SimpleProperty; import org.apache.james.util.CompletableFutureUtil; import org.apache.james.util.FluentFutureStream; +import org.apache.james.util.OptionalConverter; import org.apache.james.util.streams.JamesCollectors; import com.datastax.driver.core.BoundStatement; @@ -100,6 +101,7 @@ public class CassandraMessageDAO { private final PreparedStatement selectByBatch; private CassandraUtils cassandraUtils; private final CassandraConfiguration cassandraConfiguration; + private final Cid.CidParser cidParser; @Inject public CassandraMessageDAO(Session session, CassandraTypesProvider typesProvider, CassandraConfiguration cassandraConfiguration, @@ -115,6 +117,7 @@ public class CassandraMessageDAO { this.cassandraConfiguration = cassandraConfiguration; this.selectByBatch = prepareSelectBatch(session, cassandraConfiguration); this.cassandraUtils = cassandraUtils; + this.cidParser = Cid.parser().relaxed(); } @VisibleForTesting @@ -283,7 +286,8 @@ public class CassandraMessageDAO { return MessageAttachmentRepresentation.builder() .attachmentId(AttachmentId.from(udtValue.getString(Attachments.ID))) .name(udtValue.getString(Attachments.NAME)) - .cid(Optional.ofNullable(udtValue.getString(Attachments.CID)).map(Cid::from)) + .cid(OptionalConverter.fromGuava( + cidParser.parse(udtValue.getString(Attachments.CID)))) .isInline(udtValue.getBool(Attachments.IS_INLINE)) .build(); } http://git-wip-us.apache.org/repos/asf/james-project/blob/eaf503e6/mailbox/cassandra/src/main/java/org/apache/james/mailbox/cassandra/mail/CassandraMessageDAOV2.java ---------------------------------------------------------------------- diff --git a/mailbox/cassandra/src/main/java/org/apache/james/mailbox/cassandra/mail/CassandraMessageDAOV2.java b/mailbox/cassandra/src/main/java/org/apache/james/mailbox/cassandra/mail/CassandraMessageDAOV2.java index 6664028..b44bd8c 100644 --- a/mailbox/cassandra/src/main/java/org/apache/james/mailbox/cassandra/mail/CassandraMessageDAOV2.java +++ b/mailbox/cassandra/src/main/java/org/apache/james/mailbox/cassandra/mail/CassandraMessageDAOV2.java @@ -59,6 +59,7 @@ import org.apache.james.backends.cassandra.utils.CassandraAsyncExecutor; import org.apache.james.mailbox.cassandra.ids.BlobId; import org.apache.james.mailbox.cassandra.ids.CassandraMessageId; import org.apache.james.mailbox.cassandra.mail.utils.Limit; +import org.apache.james.mailbox.cassandra.table.CassandraMessageV1Table; import org.apache.james.mailbox.cassandra.table.CassandraMessageV2Table.Attachments; import org.apache.james.mailbox.cassandra.table.CassandraMessageV2Table.Properties; import org.apache.james.mailbox.exception.MailboxException; @@ -74,6 +75,7 @@ import org.apache.james.mailbox.store.mail.model.impl.PropertyBuilder; import org.apache.james.mailbox.store.mail.model.impl.SimpleProperty; import org.apache.james.util.CompletableFutureUtil; import org.apache.james.util.FluentFutureStream; +import org.apache.james.util.OptionalConverter; import org.apache.james.util.streams.JamesCollectors; import com.datastax.driver.core.BoundStatement; @@ -103,6 +105,7 @@ public class CassandraMessageDAOV2 { private final PreparedStatement selectHeaders; private final PreparedStatement selectFields; private final PreparedStatement selectBody; + private final Cid.CidParser cidParser; @Inject public CassandraMessageDAOV2(Session session, CassandraTypesProvider typesProvider, CassandraBlobsDAO blobsDAO, CassandraConfiguration cassandraConfiguration) { @@ -116,6 +119,7 @@ public class CassandraMessageDAOV2 { this.selectHeaders = prepareSelect(session, HEADERS); this.selectFields = prepareSelect(session, FIELDS); this.selectBody = prepareSelect(session, BODY); + this.cidParser = Cid.parser().relaxed(); } @VisibleForTesting @@ -340,7 +344,8 @@ public class CassandraMessageDAOV2 { return MessageAttachmentRepresentation.builder() .attachmentId(AttachmentId.from(udtValue.getString(Attachments.ID))) .name(udtValue.getString(Attachments.NAME)) - .cid(Optional.ofNullable(udtValue.getString(Attachments.CID)).map(Cid::from)) + .cid(OptionalConverter.fromGuava( + cidParser.parse(udtValue.getString(CassandraMessageV1Table.Attachments.CID)))) .isInline(udtValue.getBool(Attachments.IS_INLINE)) .build(); } http://git-wip-us.apache.org/repos/asf/james-project/blob/eaf503e6/mailbox/store/src/main/java/org/apache/james/mailbox/store/mail/model/impl/MessageParser.java ---------------------------------------------------------------------- diff --git a/mailbox/store/src/main/java/org/apache/james/mailbox/store/mail/model/impl/MessageParser.java b/mailbox/store/src/main/java/org/apache/james/mailbox/store/mail/model/impl/MessageParser.java index 3e7e42e..62bf74e 100644 --- a/mailbox/store/src/main/java/org/apache/james/mailbox/store/mail/model/impl/MessageParser.java +++ b/mailbox/store/src/main/java/org/apache/james/mailbox/store/mail/model/impl/MessageParser.java @@ -69,25 +69,14 @@ public class MessageParser { ContentDispositionField.DISPOSITION_TYPE_ATTACHMENT.toLowerCase(Locale.US), ContentDispositionField.DISPOSITION_TYPE_INLINE.toLowerCase(Locale.US)); private static final Logger LOGGER = LoggerFactory.getLogger(MessageParser.class); - private static final Function<String, Optional<Cid>> STRING_TO_CID_FUNCTION = new Function<String, Optional<Cid>>() { - @Override - public Optional<Cid> apply(String cid) { - try { - return Optional.of(Cid.from(cid)); - } catch (IllegalArgumentException e) { - return Optional.absent(); - } - } - }; - - private static final Function<ContentIdField, Optional<Cid>> CONTENT_ID_FIELD_TO_OPTIONAL_CID_FUNCTION = new Function<ContentIdField, Optional<Cid>>() { - @Override - public Optional<Cid> apply(ContentIdField field) { - return Optional.fromNullable(field.getId()) - .transform(STRING_TO_CID_FUNCTION) - .or(Optional.<Cid>absent()); - } - }; + + private final Cid.CidParser cidParser; + + public MessageParser() { + cidParser = Cid.parser() + .relaxed() + .unwrap(); + } public List<MessageAttachment> retrieveAttachments(InputStream fullContent) throws MimeException, IOException { DefaultMessageBuilder defaultMessageBuilder = new DefaultMessageBuilder(); @@ -197,8 +186,20 @@ public class MessageParser { } private Optional<Cid> cid(Optional<ContentIdField> contentIdField) { - return contentIdField.transform(CONTENT_ID_FIELD_TO_OPTIONAL_CID_FUNCTION) - .or(Optional.<Cid>absent()); + if (!contentIdField.isPresent()) { + return Optional.absent(); + } + return contentIdField.transform(toCid()) + .get(); + } + + private Function<ContentIdField, Optional<Cid>> toCid() { + return new Function<ContentIdField, Optional<Cid>>() { + @Override + public Optional<Cid> apply(ContentIdField input) { + return cidParser.parse(input.getId()); + } + }; } private boolean isMultipart(Entity entity) { --------------------------------------------------------------------- To unsubscribe, e-mail: server-dev-unsubscr...@james.apache.org For additional commands, e-mail: server-dev-h...@james.apache.org