JAMES-2214 Draft saving #1120

chibenwa · 2017-11-13T07:11:43Z

No description provided.

mbaechler

Some comments along the way, mostly about naming.
Overall, I'm happy to see that things are fitting quite well in existing codebase and that you managed to do that so quickly !

mbaechler · 2017-11-13T07:58:16Z

...protocols/jmap/src/main/java/org/apache/james/jmap/methods/SetMessagesCreationProcessor.java

@@ -279,8 +258,10 @@ private MessageWithId handleOutboxMessages(CreationMessageEntry entry, MailboxSe
        if (!isRequestForSending(entry.getValue(), session)) {
            throw new IllegalStateException("Messages for everything but outbox should have been filtered earlier");
        }
-        MetaDataWithContent newMessage = createMessageInOutbox(entry, outbox, session);
-        return sendMessage(entry.getCreationId(), newMessage, session);
+        MetaDataWithContent newMessage = messageAppender.createMessageInMailbox(entry, outbox, session);


the previous version was simpler to understand.

Yes but see later on: we need a valid Jmap message when saving a draft : hence I came back and did not factorize JMAP message computation.

Moreover think to the Big Picture: That extraction is ready for draft moving to outbox.

mbaechler · 2017-11-13T07:59:55Z

server/protocols/jmap/src/main/java/org/apache/james/jmap/methods/MessageAppender.java

+        this.mimeMessageConverter = mimeMessageConverter;
+    }
+
+    public MessageFactory.MetaDataWithContent createMessageInMailbox(ValueWithId.MessageWithId.CreationMessageEntry createdEntry,


appendMessageInMailbox

If you wish

mbaechler · 2017-11-13T08:01:18Z

server/protocols/jmap/src/main/java/org/apache/james/jmap/methods/MessageAppender.java

+    }
+
+    public MessageFactory.MetaDataWithContent createMessageInMailbox(ValueWithId.MessageWithId.CreationMessageEntry createdEntry,
+                                                                     MessageManager mailbox,


if it's a jmap API, why don't you take a MailboxId here ?

Well SystemMailboxes provider directly gives MessageManager.

No need to perform an extra lookup

We already have it in caller code.

what caller code ? you don't know the caller, you are creating an API.
Or maybe it's a private class used by a single consumer ?
Otherwise, please make the API as consistent as you would like to find it as a developper browsing the API.

benoit, if you want to keep this optimization you can provide the 2 methods, one with MessageId, and one with the MessageManager.

Ok for duplicated methods of course.

Ps: I don't concider this really an optimization.

It is more I have a superset of MailboxId, I wish to be calling the API with my superset of MailboxId, not doing a .getMailboxId.

mbaechler · 2017-11-13T08:01:56Z

server/protocols/jmap/src/main/java/org/apache/james/jmap/methods/MessageAppender.java

+        this.mimeMessageConverter = mimeMessageConverter;
+    }
+
+    public MessageFactory.MetaDataWithContent createMessageInMailbox(ValueWithId.MessageWithId.CreationMessageEntry createdEntry,


could you import ValueWithId so that we only read MessageWithId here ?

mbaechler · 2017-11-13T08:03:49Z

server/protocols/jmap/src/main/java/org/apache/james/jmap/methods/MessageSender.java

+        this.mailFactory = mailFactory;
+    }
+
+    public void sendMessage(Message jmapMessage,


how are the two first argument related ?

One is generated from the other.

You have 3 related things:

CreationMessage

MessageMetadata: once you saved it in store

JMAP message is used only for the enveloppe.

We shoudl have the MailFactory taking an Enveloppe instead of a full JMAP message.

mbaechler · 2017-11-13T08:18:13Z

...ng-common/src/test/java/org/apache/james/jmap/methods/integration/SetMessagesMethodTest.java

+    }
+
+    @Test
+    public void setMessageShouldRejectCreateInDraftAndOutbox() {


...CreateInBothDraftAndOutboxForASingleMessage

mbaechler · 2017-11-13T08:19:04Z

...ng-common/src/test/java/org/apache/james/jmap/methods/integration/SetMessagesMethodTest.java

+    }
+
+    @Test
+    public void setMessageShouldRequireDraftFlag() {


in any case or just in Draft mailbox ?

mbaechler · 2017-11-13T08:21:29Z

...protocols/jmap/src/main/java/org/apache/james/jmap/methods/SetMessagesCreationProcessor.java

-            MessageWithId created = handleOutboxMessages(create, mailboxSession);
-            responseBuilder.created(created.getCreationId(), created.getValue());
-
+            performCreate(create, responseBuilder, mailboxSession);


handleCreate is calling performCreate.
I think that bad naming scheme reflect the lack of proper abstraction. What was wrong with this version ?

I did not like the double check to the feature:

1 check to see if it's draft OR outbox, 1 second check to perform the feature (that was always sending).

Generally, error management is performed in a separated method. Is that a bad thing? I tend to concider separating these concerns is good.

mbaechler · 2017-11-13T08:22:39Z

...protocols/jmap/src/main/java/org/apache/james/jmap/methods/SetMessagesCreationProcessor.java

+        responseBuilder.created(created.getCreationId(), created.getValue());
+    }
+
+    private void validateDraftKeywords(CreationMessageEntry entry) {


mbaechler · 2017-11-13T08:24:00Z

...ng-common/src/test/java/org/apache/james/jmap/methods/integration/SetMessagesMethodTest.java

@@ -1396,6 +1396,40 @@ public void setMessageShouldRequireDraftInOnlyDraftMailbox() {
    }

    @Test
+    public void setMessageShouldBeCompatibleWithIsDraftProperty() {


what does it mean ?

We have two distinct way to interact with flags:

Through **is* ** properties

Through keywords object

I just want to check INBOX will be able to save a message with isDraft, as they do not support keywords yet.

I find this name explicit. Any proposal for improvment?

I propose " check INBOX will be able to save a message with isDraft,"
I'm kidding but "should be compatible" doesn't say what you explained.
So setMessageShouldAllowDraftCreationWhenUsingIsDraftProperty ?

aduprat · 2017-11-13T09:25:24Z

server/protocols/jmap/src/main/java/org/apache/james/jmap/methods/MessageSender.java

+    private Mail buildMessage(MessageFactory.MetaDataWithContent message, Message jmapMessage) throws MessagingException {
+        try {
+            return mailFactory.build(message, jmapMessage);
+        } catch (IOException e) {


Can you wrap the IOException directly in MailFactory.build ?

aduprat · 2017-11-13T09:29:47Z

server/protocols/jmap/src/main/java/org/apache/james/jmap/model/CreationMessage.java

@@ -363,6 +364,11 @@ private void assertValidFromProvided(ImmutableList.Builder<ValidationResult> err
    public boolean isIn(MessageManager mailbox) {
        return mailboxIds.contains(mailbox.getId().serialize());
    }
+
+    public boolean isInOnly(MessageManager mailbox) {


isTheOnlyOne or highlander

rouazana · 2017-11-13T10:30:18Z

server/protocols/jmap/src/main/java/org/apache/james/jmap/methods/MessageAppender.java

+    }
+
+    public MessageFactory.MetaDataWithContent createMessageInMailbox(ValueWithId.MessageWithId.CreationMessageEntry createdEntry,
+                                                                     MessageManager mailbox,


benoit, if you want to keep this optimization you can provide the 2 methods, one with MessageId, and one with the MessageManager.

rouazana · 2017-11-13T10:31:16Z

server/protocols/jmap/src/main/java/org/apache/james/jmap/methods/MessageAppender.java

+                .isInline(attachment.isIsInline())
+                .build());
+        } catch (AttachmentNotFoundException e) {
+            // should not happen (checked before)


is this comment still true?

Probably not, but it can be safely removed I assume.

rouazana · 2017-11-13T10:38:28Z

server/protocols/jmap/src/test/java/org/apache/james/jmap/methods/AttachmentCheckerTest.java

+            session))
+            .isInstanceOf(AttachmentsNotFoundException.class)
+            .matches(e -> ((AttachmentsNotFoundException)e).getAttachmentIds().containsAll(ImmutableSet.of(unknownBlobId1, unknownBlobId2)));
+    }


you have 2 negative test cases, can you add positive test cases?
assertAttachmentExistsShouldNotThrowWhenKnownBlobId
assertAttachmentsExistShouldNotThrowWhenKnownBlobIds

We would also need the mix between positive and negative.

Ps: I did not do it as I only intended to move code around.

rouazana · 2017-11-13T10:41:42Z

...ng-common/src/test/java/org/apache/james/jmap/methods/integration/SetMessagesMethodTest.java

+    }
+
+    @Test
+    public void setMessageShouldNotCheckFromWhenDraft() {


setMessages (with a s at the end), and same remark in every test case

rouazana · 2017-11-13T10:43:00Z

...ng-common/src/test/java/org/apache/james/jmap/methods/integration/SetMessagesMethodTest.java

+            "    \"setMessages\","+
+            "    {" +
+            "      \"create\": { \"" + messageCreationId  + "\" : {" +
+            "        \"from\": { \"name\": \"Me\", \"email\": \"invalid@domain.com\"}," +


can you also add a test case with an email without "@"?

rouazana · 2017-11-13T10:46:22Z

...ocols/jmap/src/test/java/org/apache/james/jmap/methods/SetMessagesCreationProcessorTest.java

@@ -294,25 +292,6 @@ public void processShouldNotSpoolMailWhenNotSavingToOutbox() throws Exception {
    }

    @Test


you don't need to add any unit test? (yes I've seen that you did a great coverage in integration tests)

With mocking?

Sorry, but it ties down to much SetMessagesCreationProcessor with lower layers, I really don't like it.

If you ask me for it, I prefer porting these tests to rely on InMemoryMailboxManager through InMemoryIntegrationResousources.

BTW now coverage with unit test will be duplication, isn't it?

That was just a question. It's not really needed as you are doing all these integration tests. Sometime you need to add some unit tests (so yes with mocking) to check special behaviour such as storage returning some exceptions.

I have coverage for the error management I introduced, and BTW extended it ;-)

I don't think we end up in that scenario.

rouazana · 2017-11-13T10:47:57Z

...ocols/jmap/src/test/java/org/apache/james/jmap/methods/SetMessagesCreationProcessorTest.java

@@ -300,6 +303,7 @@ public void processShouldNotSendWhenSavingToDrafts() throws Exception {
                .create(
                        creationMessageId, creationMessageBuilder.mailboxId(DRAFTS_ID.serialize()).build())
                .build();
+        when(mockedMailboxManager.getMailbox(any(MailboxId.class), any())).thenReturn(drafts);


rouazana · 2017-11-13T10:49:43Z

mailbox/store/src/main/java/org/apache/james/mailbox/store/StoreAttachmentManager.java

@@ -53,6 +53,11 @@ public StoreAttachmentManager(AttachmentMapperFactory attachmentMapperFactory, M
    }

    @Override
+    public boolean exists(AttachmentId attachmentId, MailboxSession session) throws MailboxException {


There is no existing test for AttachmentManager, only tests on mapper.

As testing this might require either:

Creating generic tests (AbstractAttachmentManagerTests blah)

Relying on mocking (there is a little test with mocking, but I really don't like mocking)

So I would go for the first one, if you realy want it.

no that's ok, manager is just wiring so we can go without these tests

rouazana · 2017-11-13T10:50:39Z

server/protocols/jmap/src/main/java/org/apache/james/jmap/methods/MessageAppender.java

@@ -58,7 +59,7 @@ public MessageAppender(AttachmentManager attachmentManager, MIMEMessageConverter
        this.mimeMessageConverter = mimeMessageConverter;
    }

-    public MessageFactory.MetaDataWithContent appendMessageInMailbox(ValueWithId.MessageWithId.CreationMessageEntry createdEntry,
+    public MessageFactory.MetaDataWithContent appendMessageInMailbox(CreationMessageEntry createdEntry,


the remark was about importing ValueWithId, not MessageWithId, so you keep MessageWithId.CreationMessageEntry

Sorry I don't get it. Naming CreationMessageEntry seems to convey enough meaning, I believe we can get a ride of MessageWithId

rouazana · 2017-11-13T10:51:54Z

...cols/jmap/src/main/java/org/apache/james/jmap/methods/MailboxSendingNotAllowedException.java


-    public MailboxSendingNotAllowedException(Collection<String> allowedFroms) {
+    public MailboxSendingNotAllowedException(String allowedFroms) {


s/Froms/From/

chibenwa · 2017-11-13T16:42:28Z

test this please

chibenwa · 2017-11-14T04:07:52Z

I do not succeed to reproduce CI issue locally:

CassandraSetMessagesMethodTest>SetMessagesMethodTest.attachmentsAndBodyShouldBeRetrievedWhenChainingSetMessagesAndGetMessagesWithTextBodyAndHtmlAttachment:3725 » ConditionTimeout
[6400799653078367686daca4bef60295411039a4] [ERROR]   CassandraSetMessagesMethodTest>SetMessagesMethodTest.attachmentsShouldBeRetrievedWhenChainingSetMessagesAndGetMessagesTextAttachment:3560 » ConditionTimeout

chibenwa · 2017-11-14T06:40:57Z

Still the CI error:

[a368dd37ef0c0df1ae8c9dbbe341ff438ea45035] [ERROR] Errors: 
[a368dd37ef0c0df1ae8c9dbbe341ff438ea45035] [ERROR]   CassandraSetMessagesMethodTest>SetMessagesMethodTest.attachmentsShouldBeRetrievedWhenChainingSetMessagesAndGetMessagesTextAttachment:3593 » ConditionTimeout
[a368dd37ef0c0df1ae8c9dbbe341ff438ea45035] [INFO] 
[a368dd37ef0c0df1ae8c9dbbe341ff438ea45035] [ERROR] Tests run: 1776, Failures: 0, Errors: 1, Skipped: 1

mbaechler · 2017-11-14T09:11:45Z

...ng-common/src/test/java/org/apache/james/jmap/methods/integration/SetMessagesMethodTest.java

+    }
+
+    @Test
+    public void setMessagesShouldBeCompatibleWithIsDraftProperty() {


you didn't change this method name based on my suggestion ?

mbaechler · 2017-11-14T09:15:38Z

server/protocols/jmap/src/main/java/org/apache/james/jmap/model/Envelope.java

+import com.google.common.base.Preconditions;
+import com.google.common.base.Throwables;
+
+public class Envelope {


Not sure about the choosen wording, Envelope usually includes headers and here that's not the case.

Do you have a better idea? I can not come up with a better name....

I would strip Sender and call the remaining fields "Recipients".

mbaechler · 2017-11-14T09:16:11Z

server/protocols/jmap/src/main/java/org/apache/james/jmap/send/MailFactory.java

-            LOGGER.error("Invalid mail address", emailer.getEmail());
-            throw Throwables.propagate(e);
-        }
+            Sets.union(envelope.getTo(), envelope.getCc()),


envelope.listRecipients ?

rouazana · 2017-11-14T10:19:28Z

server/protocols/jmap/src/test/java/org/apache/james/jmap/methods/AttachmentCheckerTest.java

@@ -101,4 +117,49 @@ public void assertAttachmentsExistShouldThrowWhenUnknownBlobIds() throws Mailbox
            .matches(e -> ((AttachmentsNotFoundException)e).getAttachmentIds().containsAll(ImmutableSet.of(unknownBlobId1, unknownBlobId2)));
    }

+    @Test
+    public void assertAttachmentsExistShouldNotThrowThrowWhenKnownBlobIds() throws Exception {


s/ThrowThrow/Throw/

…JMAP

This is required as recipients should not be validated when saving a draft, but only when sending it.

When we reached the code about saving message in Outbox, we already enforced the message should only be in outbox, hence that code was not needed.

…eationProcessor

Before, messageIds was stated, which was wrong. That error properties was not tested.

MailboxRightsException leads to think it concerns mailbox rights, but was raised when having rights on a delegated mailbox

…tion MailboxNotImplementedException and the "Not yet implemented" message is misleading. JMAP spec does not support, whatever version you take, saving messages in other mailboxes than Outbox or Draft. The error message name should explain that the given mailboxes were invalid. Moreover, the error message should explain which state was not met by the provided mailboxes.

…ethods

- More fluent API - Avoid retrieving full attachment content for existence checks

… an Envelope and not a full JMAP Message

…eationProcessor

We need a more readable way to concat (n) streams than chaining Stream.concat calls

mbaechler reviewed Nov 13, 2017

View reviewed changes

aduprat reviewed Nov 13, 2017

View reviewed changes

rouazana reviewed Nov 13, 2017

View reviewed changes

chibenwa force-pushed the JAMES-2214 branch from a368dd3 to 3c6fd7b Compare November 14, 2017 07:03

aduprat approved these changes Nov 14, 2017

View reviewed changes

mbaechler reviewed Nov 14, 2017

View reviewed changes

rouazana approved these changes Nov 14, 2017

View reviewed changes

chibenwa force-pushed the JAMES-2214 branch 2 times, most recently from a222f59 to feac3b1 Compare November 15, 2017 08:29

chibenwa mentioned this pull request Nov 15, 2017

James 2214 combined #1129

Closed

chibenwa added 17 commits November 15, 2017 17:58

JAMES-2214 Introduce helper class for saving and sending emails with …

4dbe064

…JMAP

JAMES-2214 Recipient validation should be done before sending

e3ffd2b

This is required as recipients should not be validated when saving a draft, but only when sending it.

JAMES-2214 Remove redundant OUTBOX checking

d324caa

When we reached the code about saving message in Outbox, we already enforced the message should only be in outbox, hence that code was not needed.

JAMES-2214 Attachment checking should be extracted from SetMessagesCr…

b0f4d06

…eationProcessor

JAMES-2214 Correct error properties when saving with wrong attachments

903ead0

Before, messageIds was stated, which was wrong. That error properties was not tested.

JAMES-2214 Fix mailbox not found invalid lambda in SetMessages::create

f85b5c6

JAMES-2214 Implement Draft saving via JMAP

fe0522a

JAMES-2214 Rename ambiguous and misleading MailboxRightsException

1e8d057

MailboxRightsException leads to think it concerns mailbox rights, but was raised when having rights on a delegated mailbox

JAMES-2214 s/validate/assert/ for SetMessageCreationProcessor check m…

264ffd7

…ethods

JAMES-2214 Add an AttachmentManager exist method

90f3fb7

- More fluent API - Avoid retrieving full attachment content for existence checks

JAMES-2214 Only one From address is supported: the user name

46e520e

JAMES-2214 s/validate/assert/ in MessageSender

2b22852

JAMES-2214 Make calls to MailFactory less ambiguous by only requiring…

a04c071

… an Envelope and not a full JMAP Message

JAMES-2214 MailFactory should not throw IOException

9f1d767

JAMES-2214 Attachment checking should be extracted from SetMessagesCr…

20a6c64

…eationProcessor

PROTOCOLS-117 Add a stream helper

5c904e1

We need a more readable way to concat (n) streams than chaining Stream.concat calls

chibenwa added 2 commits November 15, 2017 18:05

PROTOCOLS-117 Use StreamUtils::flatten

944643b

JAMES-2214 Improve Emailer and envelope design

5df4371

chibenwa force-pushed the JAMES-2214 branch from feac3b1 to 5df4371 Compare November 15, 2017 11:06

aduprat merged commit 5df4371 into linagora:master Nov 15, 2017

vttranlina pushed a commit to vttranlina/james-project that referenced this pull request Aug 15, 2022

JAMES-3771 ADR for the OpenSearch migration (linagora#1120)

8e3088a

		@@ -294,25 +292,6 @@ public void processShouldNotSpoolMailWhenNotSavingToOutbox() throws Exception {
		}

		@Test


		public MailboxSendingNotAllowedException(Collection<String> allowedFroms) {
		public MailboxSendingNotAllowedException(String allowedFroms) {

JAMES-2214 Draft saving #1120

JAMES-2214 Draft saving #1120

Conversation

chibenwa commented Nov 13, 2017

mbaechler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chibenwa Nov 13, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chibenwa Nov 13, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chibenwa Nov 13, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chibenwa commented Nov 13, 2017

chibenwa commented Nov 14, 2017

chibenwa commented Nov 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chibenwa Nov 13, 2017 •

edited

chibenwa Nov 13, 2017 •

edited

chibenwa Nov 13, 2017 •

edited