Subject: [Jmap] Review of draft-ietf-jmap-core-09

From: Stephan Bosch <[email-masked]>
Date: Tue, 6 Nov 2018 00:30:24 +0100

Hi,

Here are some of the comments and suggestions I collected while reading 
draft-ietf-jmap-core-09. Much of it is editorial, but there are some 
substantive comments as well.

I have no more comments beyond these and I like how my previous comments 
were processed. However, I am mostly a protocol guy in this case, and I 
don't have much experience with the actual implementation of (IMAP) 
storage, mail thread handling, indexing, caching, state handling, 
searching, sorting etc. So, such concerns will not be covered well by my 
reviews.

As before, text from the document itself is pasted here as-is. My 
comments are prefixed with "-> ".

I am not sure I'll be able to provide a final review of mail-09 as well 
before the meeting, nor am I sure I can attend the meeting in the first 
place (remotely).

Regards,

Stephan.

## Section 2:

-> The description of the *state* field could use reference to Section 3.3.

## Section 3.2, 3.3:

-> The descriptions of the *createdIds* field can use a reference to
Section 5.8.

## Section 3.3:

-> The description of the sessionState field can use a reference to 
Section 2.

## Section 3.5.2:

-> The "accountNotSupportedByMethod" error seems backward; the method is not
supported by the account

## Section 4:

-> Is it really useful to have a completely random (and arbitrarily complex)
content for the echo request that the server must return? Why not just a 
single
string field?

## Section 5.1:

The following additional error may be returned instead of the _Foo/
get_ response:

"requestTooLarge": The number of _ids_ requested by the client
exceeds the maximum number the server is willing to process in a
single method call.

-> What if the _ids_ field is actually a back reference to something 
that yields
a lot of ids (i.e. more than the limit)? This is pretty much the complicated
example in Section 3.6. Is there some means to limit the number of 
results from
the back reference (or specify an offset, to get results in multiple 
requests)?
Or should the client then (somehow) use a /query instead? That could be
difficult for this example. Or am I just making this more complicated 
than it
needs to be?

## Section 5.3:

Some record objects may hold references to others (foreign keys).
When records are created or modified, they may reference other
records being created _in the same API request_ by using the creation
id prefixed with a "#".  The order of the method calls in the request
by the client MUST be such that the record being referenced is
created in the same or an earlier call. <and onwards>

-> This can use an explicit reference to Section 3.2/3.3.

## Section 5.5, 5.6:

-> Why do these methods echo back certain (potentially quite complex) fields
back to the client?

## Section 7.2:

-> The TypeState object could use a reference to the previous section.

Any other "4xx" or "5xx" response code MUST be considered a *permanent 
failure*
and the push subscription SHOULD be destroyed.
-> How would a client get to know about this? Could a useful error message
somehow be retrieved by the client?

## Section 7.3:

Refer to the JMAP Session resource section of this spec for details on 
how to
get the URL for the event-source resource.
-> This could use an explicit section reference

---

From: "Neil Jenkins" <[email-masked]>
Date: Tue, 06 Nov 2018 02:45:46 -0500

Thanks for the further review Stephan. 
 
> -> The description of the sessionState field can use a reference to  
 
All sounds reasonable, done. 
 
> -> The "accountNotSupportedByMethod" error seems backward; the method is not 
 
Yeh, I guess it's a bit more the other way. The advantage of the current construction is all those errors beginning with "account" look similar, and probably mean a similar thing in the end (your account data is out-of-date, otherwise you wouldn't have made this request). I'm happy to change, but don't see a great need to. 
 
> single string field? 
 
I don't know about useful but I actually think echoing back the whole arguments object is simpler. Presumably you will have parsed the JSON into some representation using some standard library; you then just put this representation straight into the output. If anything, forcing it to have a single string argument would be more complex as you'd have to then add code for checking that the object structure matched this rather than just passing it through. 
 
> that yields a lot of ids (i.e. more than the limit)? 
 
Then this would be rejected by the server; as the spec states, the back reference is just a syntactical substitution, the method is then processed as normal. 
 
>  Is there some means to limit the number of results from 
 
Presuming your back reference is to a query you give a limit on the number of results to return at a time. When fetching the messages that belong to a thread via a back reference, yes, you don't know exactly how many results there will be. But from our service I can tell you that for most users the mean number of messages in a thread is slightly over 1, so you are unlikely to run into this in practice, and even if you do (for example opening some mailing list archives might do it), the client should handle the error and re-request in smaller batches. The previous method call (to get the threads and the list of message ids in the threads) will still have succeeded, so the client has all the information it needs to then request the messages themselves in smaller chunks in further requests. 
 
> -> This can use an explicit reference to Section 3.2/3.3. 
 
Added. 
 
> -> Why do these methods echo back certain (potentially quite complex) fields 
 
The original idea was that it gave you the full information on what the query results were for. But you're right, the client already has this information from the request so it's a waste of bandwidth to send them back again. I've removed the sort/filter/upToId arguments from the responses to these methods. I've kept oldQueryState (which mirrors /changes oldState) because it's small and it's very useful when looking at the network traffic to be able to see these together to see which states the response is taking you from and to. 
 
> -> The TypeState object could use a reference to the previous section. 
 
Added. 
 
> failure* and the push subscription SHOULD be destroyed. 
 
The only way they can know is to check the push subscriptions and see that they don't have one. I guess we could instead introduce an error state to the PushSubscription object, although I'm not sure exactly what we would want to pass back or how useful it would be to the client. We could pass the HTTP error code? And maybe the response text? Probably don't want to pass back the whole body (we don't even know what media type the body is going to be for an error). We'd probably still want the server to be able to remove subscriptions if they are left in the error state for a while. 
 
 Do people think this would be worthwhile? 
 
> -> This could use an explicit section reference 
 
Added. 
 
I also read through both of the specs myself yesterday and fixed a number of nits. I will publish new drafts today incorporating those changes and the fixes for your comments. 
 
Cheers, 
Neil.