Subject: [Jmap] AD review of draft-ietf-jmap-core-13

From: Alexey Melnikov <[email-masked]>
Date: Tue, 15 Jan 2019 13:27:46 +0000

Hi,

This is my delayed review of the document. Most of the issues are minor, 
so I didn't think they were worth delaying IETF LC for.

In Section 2:

o *primaryAccounts*: "String[Id]" A map of specification URIs

-- Definition of Id doesn't allow for ":" or "/" used in URIs. Does it 
need fixing?

In Section 5.5:

       *  *collation*: "String" (optional; default is server-dependent)
          The identifier, as registered in the collation registry defined
          in [RFC4790], for the algorithm to use when comparing the order
          of strings.  The algorithms the server supports are advertised
          in the capabilities object returned with the JMAP Session
          object.  If omitted, the default algorithm is server-dependent,
          but:

          1.  It MUST be unicode-aware.

          2.  It SHOULD have reasonable default behavior for many

              languages when the user's language is unknown.

What exactly does this mean and how can this be tested for interoperability?

          3.  It MAY be selected based on out-of-band information about

              the user's language/locale.

You might want to mention your recently added text about use of 
Accept-Language here.

          4.  It SHOULD be case-insensitive where such a concept makes

              sense for a language/locale.

You might want to point to Section 5.2.3 of RFC 8264 here.

In Section 5.7: description before the response doesn't mention keyword 
"video", but the request included both "video" and "music".

In Section 6.2:

Is it worth recommending use of RFC 8246 ("

  HTTP Immutable Responses") here?

6.3.  Blob/copy

    The *SetError* may be any of the standard set errors that may be
    returned for a _create_. In addition, the "notFound" SetError error
    may be returned if the blobId to be copied cannot be found.

    The following additional errors may be returned instead of the _Blob/
    copy_ response:

    "fromAccountNotFound": The _fromAccountId_ included with the request
    does not correspond to a valid account.

This is hard to read and your earlier text was better. Can you maybe 
talk about "notFound" and "fromAccountNotFound" together?

In Section 9.4.5:

"Intended use"should mention "placeholder", which is used in 9.4.7.

In 9.4.6/9.4.7: use RFC XXXX instead of "this document". You can also 
just reference draft-ietf-jmap-core reference here, I think RFC Editor 
will understand that.

Best Regards,

Alexey

---

From: "Neil Jenkins" <[email-masked]>
Date: Thu, 17 Jan 2019 00:12:31 -0500

> -- Definition of Id doesn't allow for ":" or "/" used in URIs. Does it need fixing?

No, this is correct. The map is of type `String` to `Id`. The keys are specification URIs, which is why this is type `String`. The values are account ids, which is why the value type of the map is `Id`.

> What exactly does this mean and how can this be tested for interoperability?

Yeh, this is a bit wishy washy. It's trying to say the ordering should be as unsurprising as possible for as large a number of languages as possible. I agree this is not a sufficiently strong requirement to test inter-operably, so I have removed this text and instead just noted that the two recommended algorithms provide reasonable behaviour for a large number of languages.

> You might want to mention your recently added text about use of Accept-Language here.

Sure. I've changed this to:

*It MAY be selected based on an Accept-Language header in the request (as defined in [@!RFC7231] section 5.3.5), or out-of-band information about the user's language/locale.***

> You might want to point to Section 5.2.3 of RFC 8264 here.

OK. I've changed this to:

*It SHOULD be case-insensitive where such a concept makes sense for a language/locale. Where the user's language is unknown, it is RECOMMENDED to follow the advice in section 5.2.3 of [@!RFC 8264].*

> In Section 5.7: description before the response doesn't mention keyword "video", but the request included both "video" and "music".

I have fixed the text here.

> Is it worth recommending use of RFC 8246 ("**

> HTTP Immutable Responses") here?

Yes, that's reasonable. I have done this.

> This is hard to read and your earlier text was better. Can you maybe talk about "notFound" and "fromAccountNotFound" together?

Hmm, I'm not sure how best to clarify this. The notFound error is at the set level, whereas the fromAccountNotFound is at the method level so it's hard to combine. I've made some small tweaks to clarify this and reference the standard set-level errors:

*The *SetError* may be any of the standard set errors that may be returned for create, as defined in section 5.3. In addition, the `notFound` SetError error may be returned if the blobId to be copied cannot be found.***
**
*The following additional method-level error may be returned instead of the Blob/copy response:***
**
*`fromAccountNotFound`: The fromAccountId included with the request does not correspond to a valid account.*

Is that sufficient?

> "Intended use"should mention "placeholder", which is used in 9.4.7.

Thanks, fixed.

> In 9.4.6/9.4.7: use RFC XXXX instead of "this document". You can also just reference draft-ietf-jmap-core reference here, I think RFC Editor will understand that.

Right, fixed.

I have now published new drafts with the fixes.

Cheers,
Neil.

---

From: Alexey Melnikov <[email-masked]>
Date: Wed, 23 Jan 2019 11:15:41 +0000

Hi Neil,

On 17/01/2019 05:12, Neil Jenkins wrote:
> No, this is correct. The map is of type |String| to |Id|. The keys are 
> specification URIs, which is why this is type |String|.. The values 
> are account ids, which is why the value type of the map is |Id|.
>
Never mind then :-).
> Hmm, I'm not sure how best to clarify this. The notFound error is at 
> the set level, whereas the fromAccountNotFound is at the method level 
> so it's hard to combine. I've made some small tweaks to clarify this 
> and reference the standard set-level errors:
>
> /The *SetError* may be any of the standard set errors that may be 
> returned for create, as defined in section 5.3. In addition, the 
> |notFound| SetError error may be returned if the blobId to be copied 
> cannot be found./
>
> /The following additional method-level error may be returned instead 
> of the Blob/copy response:/
>
> /|fromAccountNotFound|: The fromAccountId included with the request 
> does not correspond to a valid account./
>
> Is that sufficient?

I think this would be fine.

Best Regards,

Alexey