Re: [core] Iotdir last call review of draft-ietf-core-senml-etch-05

[Ari Keränen <[e-mail]>]

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 <[e-mail]> wrote:
> 
> Reviewer: Matthias Kovatsch
> Review result: 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.)

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.
 
> ## Technical comments
> 
> * P4 (3.1): I am missing assertions such as "Values in a Fetch Record MUST be
> ignored."

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".

> * P5 §3: The record must not be added when the value is null. (behavior not
> 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."

> * P7: "Windows Clipboard Name" --> Microsoft and for instance HTML spec use
> "Windows Clipboard Format"
>  * 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
>  this purpose, no?

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
>    Formats; are there any?

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.

> ## 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?!")

Indeed. I'm not aware of use case for using Patch with LwM2M exec resources, but can double check this.
 
> ## 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...

Fixed.
 
> * 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"

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.

> * 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."

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.
 
> * 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"

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."

> * 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".

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