Subject: [Jmap] Review: draft-ietf-jmap-mail-10

From: "Bron Gondwana" <[email-masked]>
Date: Thu, 15 Nov 2018 01:45:58 -0500

here's my belated readthrough and comments on jmap-mail.
* *
*1.3.1 **urn:ietf:params:jmap:mail* 
 
maxMailboxDepth - do you mean "one more than the maximum number of ancestors"?  Aka, "1" means no tree, all toplevel.
 
> *maxSizeMailboxName*: "PositiveInt" The maximum length, in (UTF-8)
octets, allowed for the name of a mailbox.  This MUST be >= 255.
 
This might actually be quite hard for some servers. 255 is a long name! I'd recommend a lower minimum of say 100 octets, though recommend that servers allow more. 
 
*1.3.2 **urn:ietf:params:jmap:submission**  *
 
> By default, the JMAP server should show only known safe-
to-expose EHLO capabilities in this field, and hide EHLO
capabilities that are only relevant to the JMAP server.
 
What does this actually mean? I find this sentence confusing about relevance. 
* *
*1.5 Push* 
 
> In addition, servers MUST support a psuedo-type called
"EmailDelivery" in the push mechanisms.  The state string for this
MUST change whenever a new Email is added to the store, but SHOULD
NOT change upon any other change to the Email objects.
 
Is that really something that should be a MUST? I guess it can just be a copy of "Email" if you can't tell if something was a new message. 
 
*2. Mailbox* 
 
This should probably cross-reference RFC8474 OBJECTID at least as an informative reference. 
 
isSubscribed should reference the IMAP property that it's cloning. 
 
*3. Mailbox/set* 
 
Do we also want an onDestroyRemoveChildren argument to match onDestroyRemoveMessages? 
 
*4.1.4 Body parts *
 
{I admit I skim-read over the email format parsing stuff - I've read it before,and it all seems sane}, except maybe: 
 
*preview*: see the discussion in [email-masked] about IMAP previews and making them consistent. I can see an argument for making the size of this be in characters rather than octets. I do feel strongly that we should make the limits exactly consistent between JMAP and IMAP, since we have that opportunity right now. 
 
There also needs to be text here about the flexible immutability of this field. It's derived and the algorithm might change, but the value is still considered "correct" and doesn't need to be updated by the client. 
* *
*4.2 Email/get* 
 
> Where a specific header is requested as a property, the
capitalization of the property name in the response MUST be identical
to that used in the request.
 
Does this mean that a request for "From:asRaw" and "from:asRaw" returns both of them separately? 
 
*4.4.1: Filtering* 
 
>    o  *inMailbox*: "String" A mailbox id.  An email must be in this
      mailbox to match the condition.

   o  *inMailboxOtherThan*: "String[]" A list of mailbox ids.  An email
      must be in at least one mailbox not in this list to match the
      condition.  This is to allow messages solely in trash/spam to be
      easily excluded from a search.
 
Just a reminder to change the "String" here to "Id" per the core changes... 
 
>    o  *header*: "String[]" The array MUST contain either one or two
      elements.  The first element is the name of the header field to
      match against.  The second (optional) element is the text to look
      for in the header field value.  If not supplied, the message
      matches simply if it _has_ a header field of the given name.
 
This looks like a pain to use because you can only do one per filtercondition, but I guess that's the same as the keyword items above. 
 
*4.8. Email/import* 
 
>    The _Email/import_ method adds [RFC5322] messages to the set of
   emails in an account.  The messages must first be uploaded as files
   using the standard upload mechanism.  It takes the following
   arguments:
 
I think this should say "must first be uploaded as blobs" to be consistent. 
 
Same remember to update mailboxIds type map applies. I'd be tempted to define a type for keywords as well, and have "keywords: Keyword[Boolean]" in the spec just to memoralise that it's a restricted string type. 
 
*4.9 Email/parse* 
 
Just echoing my comments in the core review about the difference between  
>    o  *created*: "String[Email]" A map of the creation id to an object
      containing the _id_, _blobId_, _threadId_ and _size_ properties
      for each successfully imported Email.
   o  *notCreated*: "String[SetError]" A map of creation id to a
      SetError object for each Email that failed to be created.  The
      possible errors are defined above.
 
and 
 
>    o  *parsed*: "String[Email]|null" A map of blob id to parsed Email
      representation for each successfully parsed blob, or "null" if
      none.
> 
>    o  *notParsable*: "String[]|null" A list of ids given that
      corresponded to blobs that could not be parsed as emails, or
      "null" if none.
 
If feels like the types of response to different methods and the null vs empty behaviour is inconsistent and weird - they should all be the same, and possibly the lists of ids should all be maps of { id : true } for deletion, etc - just because the ordering doesn't matter, and lists imply ordering. 
 
*5. Search snippets* 
 
Some language about caching might be required here? Basically immutability along the same lines as preview (expected to remain correct, but bytes might change as implementation is updated) 
 
*7. Email submission* 
 
Why does this include threadId when you can always get it with a backreferences fetch against emailId? Feels like duplication of data for dubious benefit. I assume it's for hooking submissions into the display for their matching thread, but a backreference lookup on fetch would get you that too. Ahh yeah, handy for filtering I guess... 
 
*7.5 EmailSubmission/set* 
 
Given that we're updating Emails with side effect subrequests from this, should there be a way to latch this against the Email state? ifInEmailState or something? 
 
... 
 
That'll do. Mostly just tiny editorial nits. 
 
Bron. 
 
-- 
 Bron Gondwana, CEO, FastMail Pty Ltd 
 [email-masked]

---

From: "Neil Jenkins" <[email-masked]>
Date: Thu, 22 Nov 2018 23:58:41 -0500

> maxMailboxDepth - do you mean "one more than the maximum number of ancestors"?  Aka, "1" means no tree, all toplevel.
 
Yes, good catch.
 
> What does this actually mean? I find this sentence confusing about relevance. 
 
I've changed it to: *By default, the JMAP server should hide EHLO capabilities that are to do with the transport mechanism and thus are only relevant to the JMAP server (for example PIPELINING, *CHUNKING, *or STARTTLS).*
 
Is that clear?
 
> Is that really something that should be a MUST? I guess it can just be a copy of "Email" if you can't tell if something was a new message. 
 
Yes, this is trivial and spec compliant, and will result in correct client behaviour at the expense of some efficiency. I think therefore it's better to make it a MUST than to force the client to have two pathways to handle whether it's there or not.
 
> This should probably cross-reference RFC8474 OBJECTID at least as an informative reference. 
 
I've added an *Ids* subsection to the intro and written this: 
 
*If a JMAP Mail server also provides an IMAP interface to the data and supports [@!RFC8474] IMAP Extension for Object Identifiers, the ids SHOULD be the same for mailbox, thread, and email objects in JMAP.** *
 
Is that what you were looking for, or something more?
 
> Do we also want an onDestroyRemoveChildren argument to match onDestroyRemoveMessages? 
 
I would say no. It's much easier for the client to explicitly destroy the children mailboxes than the children messages, and we'd have to be careful about the interaction of this with rate limits, with destroying messages inside the child folders and the ordering and locking that might involve, etc. I think it would add quite a bit of complexity for not a lot of gain. 
 
> *4.1.4 Body parts* 
 
I agree; I'm not sure if we have a final conclusion there, but when we do I'll make sure this is the same.
 
> Does this mean that a request for "From:asRaw" and "from:asRaw" returns both of them separately? 
 
Yes, although that wouldn't be a very useful request to make.
 
> *7. Email submission* 
>  
> Why does this include threadId … Ahh yeah, handy for filtering I guess... 
 
Exactly.
 
> Given that we're updating Emails with side effect subrequests from this, should there be a way to latch this against the Email state? ifInEmailState or something? 
 
I don't think this is needed. It's only operating on immutable properties of the email so it's of limited utility, and would add considerable complexity again. 
 
Neil.

---

From: "Bron Gondwana" <[email-masked]>
Date: Mon, 26 Nov 2018 17:26:15 -0500

> I've changed it to: *By default, the JMAP server should hide EHLO capabilities that are to do with the transport mechanism and thus are only relevant to the JMAP server (for example PIPELINING, *CH…
 
Yep, that's good. 
 
> Yes, this is trivial and spec compliant, and will result in correct client behaviour at the expense of some efficiency. I think therefore it's better to make it a MUST than to force the client to h…
 
Makes sense. 
 
> *If a JMAP Mail server also provides an IMAP interface to the data and supports [@!RFC8474] IMAP Extension for Object Identifiers, the ids SHOULD be the same for mailbox, thread, and email objects in JMAP.*** 
>  
 
That's fine - just a cross reference to make people aware that it exists and should be congruent. 
 
>> *3.. Mailbox/set* 
 
Ok, fair. 
 
> I agree; I'm not sure if we have a final conclusion there, but when we do I'll make sure this is the same. 
 
This is the one thing which is maybe blocking us right now in terms of consistency... 
 
> I don't think this is needed. It's only operating on immutable properties of the email so it's of limited utility, and would add considerable complexity again. 
 
So the main question is whether the email still exists, or whether it's still in the same folder. 
 
AKA: can we guarantee to avoid duplicate sending? 
 
I guess you can use EmailSubmission state to make sure that multiple submissions don't happen concurrently somehow. 
 
Bron.

---

From: "Neil Jenkins" <[email-masked]>
Date: Mon, 26 Nov 2018 17:29:34 -0500

> AKA: can we guarantee to avoid duplicate sending? 
 
The EmailSubmission type's state is what you would use to prevent duplicate sending, so I still don't think we need a separate ifInEmailState.
 
Neil.