Subject: Iotdir last call review of draft-ietf-core-senml-etch-05

From: Matthias Kovatsch via Datatracker <[email-masked]>
Date: Mon, 02 Sep 2019 06:07:54 -0700

Reviewer: Matthias Kovatsch
Review result: Ready with Nits

Dear authors and list members

Here is my review for draft-ietf-core-senml-etch-05 from the IoT perspective.

## Summary

draft-ietf-core-senml-etch-05 defines new media types and their semantics for
two new SenML patch document formats (JSON and CBOR, resp.). The complexity
added to implementations that can already handle SenML is marginal and
straight-forward. Hence, I do not see any issue for constrained devices. The
explicit media types help in IoT scenarios, where machines communicate with
machines.

There are a few minor issues that can be solved by the authors alone. Hence, I
marked the result as "Ready with Nits".

It would be good to get the help from an expert on Windows Clipboard Formats
and Macintosh Uniform Type Identifiers, as no good guidelines are available to
check these IANA considerations. (This issue appears to be recurrent also for
other specs.)

## Technical comments

* P4 (3.1): I am missing assertions such as "Values in a Fetch Record MUST be
ignored."
  * What should happen when a Patch Record does not have a value?

* P5 §3: The record must not be added when the value is null. (behavior not
described formally enough)

* P7: "Windows Clipboard Name" --> Microsoft and for instance HTML spec use
"Windows Clipboard Format"
  * Okay, the sting itself is the Windows Clipboard Format Name...
  * The long string with spaces ("SenML FETCH/PATCH format") is a bit weird for
  this purpose, no?
    * I also had the problem to find proper guidelines for Windows Clipboard
    Formats; are there any?
  * No Macintosh Uniform Type Identifier?

## Additional comment

* As already discussed with one of the authors, an implication for LwM2M is
probably that these patch documents must not be used with Executable Resources
(one might try to execute multiple resources at once with a PATCH method). The
application of a Patch Pack is then not idempotent anymore. Furthermore, it is
unclear what the value should be when the LwM2M Executable Resource does not
take arguments. * If executing multiple resources atomically is an important
use case, I think we need another iteration to deal with the state vs RPC issue
("use PATCH to call function(s) without arguments by giving a new state?!")

## Editorial comments

* P1 §1 (Abstract), P2 last §: "iPATCH, PATCH, and FETCH" --> "FETCH, PATCH,
and iPATCH"
  * It is easier on the brain if the order is kept consistent...

* P2 §6: "hence full name" --> "hence the unique identifiers" ?
  * RFC 8428 does not define or contain "full name", but "globally unique
  identifier for the resource"

* P3 §1: "The semantics of the ..."
  * Creates question about semantics for FETCH
  * Better to reverse sentences and start with "The rest of the document uses
  the term "(i)PATCH" to refer to both methods, as the semantics of the new
  media types are the same for the CoAP PATCH and iPATCH methods."

* P3 §1: ", that can be used with the" --> ", which ..."
* P3 §3: "Also the following ..." --> to many "also", just "The following ..."

* P4 §2 (3.1): "... when resolved, match resolved names" --> "identifiers"
  * names when resolved are resolved names, hence unclear what is compared
  * P2 calls them "full names"
  * See above, should be something like "globally unique identifier for the
  resource"
* P4 §8: Add example for records with name and time
  * Would be good to quickly show what "resolved form of records" means
* P4 §9 (3.2): Add statement that SenML patch documents are always idempotent,
hence PATCH and iPATCH are equivalent?
  * Basically move the last sentence to the beginning and give explanation for
  "(i)PATCH".

* P5 §2: "When the name" --> "When the resolved name" ?

Kind regards,
Matthias

---

From: "Eric Vyncke (evyncke)" <[email-masked]>
Date: Wed, 4 Sep 2019 17:00:15 +0000

Thank you very much Matthias for the time spent on this extensive review.

I appreciate

-éric

On 02/09/2019, 15:09, "ietf on behalf of Matthias Kovatsch via Datatracker" <[email-masked] on behalf of [email-masked]> wrote:

    
    
    ## Summary

    
    
    ## Technical comments

    
    
    ## Additional comment

    ## Editorial comments

    

    
    
    Kind regards,
    Matthias

---

From: Ari Keränen <[email-masked]>
Date: Sun, 8 Sep 2019 18:57:50 +0000

Thank you Matthias for the thorough review! I have prepared a PR to accommodate your comments:
https://github.com/core-wg/senml-etch/pull/9/files

Please see also below comments and discussion on each issue.

> On 2 Sep 2019, at 16.08, Matthias Kovatsch via Datatracker <[email-masked]> wrote:
> 
[...]
> check these IANA considerations. (This issue appears to be recurrent also for

I agree; I used here the same pattern we used for the SenML RFC (which I think we copied from some older RFC or W3C spec), but indeed better guidance in general would be useful.
 
> * P4 (3.1): I am missing assertions such as "Values in a Fetch Record MUST be

Actually this applies to all fields other than (base)name and time. I added a note to end of 3.1:
"All other Fetch Record fields than name, base name, time, and base time MUST be ignored."

I also noted this difference to SenML (lack of values) in the intro.

>  * What should happen when a Patch Record does not have a value?

Good catch! There should be either value or sum field in all Patch records; otherwise the resulting record would not be valid SenML record. I added following to 3.2: "All Patch Records MUST contain at least a SenML Value or a Sum field.  A Patch Pack with invalid Records MUST be rejected."

I noticed also that the first sentence of 3.2 was a bit misleading; I changed that from "The (i)PATCH method can be used to change the values of SenML Records" to "[...] change the fields of SenML Records".

> described formally enough)

Changed this:

OLD: If a Patch Record has a value ("v") field with value null, the matched Record (if any) is removed from the Pack.
NEW: If a Patch Record has a value ("v") field with value null, it MUST NOT be added but the matched Record (if any) is removed from the Target Pack.

I also added definition of Target Pack: "A SenML Pack that is a target for a Fetch or Patch operation."

>  * Okay, the sting itself is the Windows Clipboard Format Name...

That naming convention was used also by W3C specs (and now SenML RFC) with media type registrations.

>  * The long string with spaces ("SenML FETCH/PATCH format") is a bit weird for

Spaces seem to be OK here: for example SVG Image has clipboard format name "SVG Image". But if there's better guidance available, I'd be happy to follow that.

>    * I also had the problem to find proper guidelines for Windows Clipboard

I'm not aware of any and didn't find any good sources based on quick googling.

>  * No Macintosh Uniform Type Identifier?

There are actually Mac UTIs in sections 6.2 and 6.3.

> ("use PATCH to call function(s) without arguments by giving a new state?!")

Indeed. I'm not aware of use case for using Patch with LwM2M exec resources, but can double check this.
 
>  * It is easier on the brain if the order is kept consistent...

Fixed.
 
>  identifier for the resource"

Good point. 8428 also uses simply "name" to refer to the "unique ID that results in the concatenation of the name and base name fields". I think using "name" is less confusing in this context so I would suggest changing "full name" to just "name". But would be great to hear views of others on this too.

>  media types are the same for the CoAP PATCH and iPATCH methods."

Fixed.
 
> * P3 §1: ", that can be used with the" --> ", which ..."

Fixed.

> * P3 §3: "Also the following ..." --> to many "also", just "The following ..."

Changed to "The following additional terms" to make it clear this adds (and does not contradict) to the previous terms.
 
>  * See above, should be something like "globally unique identifier for the

To make this more clear I'd suggest adding "..match resolved names in a *Target* SenML Pack". Alternatively we could go for the "(globally) unique IDs" way.

> * P4 §8: Add example for records with name and time

Added.

>  * Would be good to quickly show what "resolved form of records" means

I added a short example of this to the end of the SenML FETCH section: "In resolved form the SenML name in the example above becomes '2001:db8::2/3311/0/5850'. Since there is no base time in the Pack, the time in resolved form is equal to the time in the example."

>  * Basically move the last sentence to the beginning and give explanation for

IMHO, moving the last sentence to the beginning would make the flow of the text a bit weird. But I suggest changing the last sentence of the paragraph into: "Application of Patch Packs is idempotent; hence PATCH and iPATCH methods for SenML Packs are equivalent."
 
> * P5 §2: "When the name" --> "When the resolved name" ?

Fixed. 

Thank you once more for the review!

Cheers,
Ari