FETCH and PATCH with Sensor Measurement Lists (SenML)
RFC 8790

Thank you for addressing my DISCUSS items.

--[ old comments

(3) Section 3.1.  A few questions about how general purpose of a query language is possible with the FETCH:

-- (To confirm based on the text “The SenML Records are selected by giving a set of names that …”) Does a name always have to be in the Fetch Request (i.e., ‘at least one “n” MUST be in the Fetch Request’)?  

Using the third example in Section 5.1.2 of RFC8428 as a source:

-- The current text mentions that names and time can be in the Fetch Pack request.  Can other fields also be used as part of the criteria, say “v”?  For example, would ‘{“n”:” urn:dev:ow:10e2073a01080063”, “v”:”21.5”}’ return the three name+time records where the value is 21.5?

(4) Section 3.1. If there is no match for a Fetch, how is that signaled back in the response?  Is there any guidance to provide on error handling via CoAP error codes?

(5) Editorial Nits:

-- Section 3.1.   s/When SenML Records contain also time values/When SenML Records also contain time values/

Firstly, thank you for writing this, and also thanks to Carlos Pignataro for the OpsDir review.

I have a question and a few nits which might be worth addressing if you are making other edits:

Question:
1: The text in Section 4 feels quite hand-wavy / terse, and I don't think gives sufficient guidance to actually use this.
e.g: What takes precedence? Do I refer to a specific record (using fragment identification) and then apply the FETCH / PATCH to that? Or do I use fragment identification to refer to records what have been PATCHed? 
As might be clear from the above, I'm not a CoRE person, so I'll be happy to accept "Your question makes no sense, this will be blindingly obvious to anyone who's actually implementing this...." :-)



Nits:
1:  Target Record:  A Record in a SenML Pack that is matching the
s/is matching/matches/

2: The names for a Fetch Pack are given using the SenML "name" and/or "base name" Fields.
I *think* that in this case "fields" would be better than "Fields" - you seem to be using the term fields generically in this case.

Thank you very much for the time spent on this document.

Please address all the comments in:
https://datatracker.ietf.org/doc/review-ietf-core-senml-etch-05-iotdir-lc-kovatsch-2019-09-02/

Thanks for addressing my DISCUSS points.

I agree with the Gen-ART reviewer and Warren that Section 4 is not specified enough. It's not clear what "analogously applied" means.

Please respond to the rest of the Gen-ART review.

It's a small thing, and not worth putting in as a DISCUSS, but please DO make this change:
Please change the registration templates in Sections 6.2 and 6.3 to accurately match the template in Section 5.6 of RFC 6838.
Thanks.

Adam raises some good points, and I'm looking forward to seeing the
ensuing discussion.

Just to double-check: these media types/methods are not suitable for use
with streaming lists (SenSML)?

I'll also echo the concerns of the genart reviewer about having a note
in the body of the document about the trailing "_" behavior of key names
for extensibility purposes.  I was going to ask for more detail of how this
works until I checked RFC 8428, but am only assuming the intent is to
"do what 8428 does" -- we should be explicit about it.

Section 4

As for several other reviewers  I'm not entirely sure that I understand the fragment case for
fetch/patch records.  These records (the request body) are themselves
selectors, so a client that wanted to consider just a subset of the
records could just send the (appropriately resolved for base values)
subset of the records in the request to obtain the selected set of
records from the resource instead of requesting the "full" set (from the
request body) and then selecting from the response via the fragment.
So, are we just specifying the fragment semantics out of a sense of
completeness, or are there expected to be cases where we still want to
retrieve records from the server just to "throw them away" at the client
side?  It is perhaps plausible to imagine doing so with (i)Patch, in
order to effect a set of changes but only retain the results for a
subset of them, but I'm having a harder time coming up with a case for
fetch fragments.  The best I can do so far is some (as-yet-to-me)
hypothetical case where the selector is easier to write with multiple
fetch records (e.g., for getting base values) but not all of them are
relevant for the desired computation.

Section 5

   In FETCH and (i)PATCH requests, the client can pass arbitrary names
   to the target resource for manipulation.  The resource implementer
   must take care to only allow access to names that are actually part
   of (or accessible through) the target resource.

I am somewhat inclined to think that we should say a bit more about
explicitly sanitizing the input names in addition to the current "take
care" language, to remove or escape any input that could be interpreted
with special meaning by the local system.

   If the client is not allowed to do a GET or PUT on the full target
   resource (and thus all the names accessible through it), access
   control rules must be evaluated for each record in the pack.

Does it matter if we think about the ACL as being applied to records in
the fetch/patch pack vs. the resource's pack?

Just one minor comment: I guess it okay to use IPSO dimmable light smart object as one real life example, however, one could also just use a made-up generic example instead as I guess it's often done in RFCs.