Re: [core] Short review of SenML fetch/patch

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

Hi Christian et al,

Thank you once more for the thorough review Christian!

After discussing the media type issue with various CoRE and OMA DM 
folks, using new media types for the SenML FETCH/PATCH seems to be a 
good approach. Hence I'm proposing we'll go that way in the next version 
of the draft (see -00 draft [1] how that could look like in practice). 
This should address most of your remaining comments below.

We still need to decide on how to indicate delete operation though. "v: 
null" and "vdel: true" seem to be our options. Some pros and cons are 
discussed in the original e-mail below. Input on this is very welcome.


Cheers,
Ari

[1] https://tools.ietf.org/html/draft-keranen-core-senml-fetch-00#section-5

On 28/09/2018 1.07, Ari Keränen wrote:
> Thank you for the review Christian!
> 
> See inline.
> 
>> On 27 Sep 2018, at 11.59, Christian Amsüss <[e-mail]> wrote:
>>
>> Hello authors,
>>
>> as requested during yesterday's interim meeting, I've had a look at
>> senml-fetch again:
>>
>> Fetch
>> =====
>>
>> Fetching by time
>> ----------------
>>
>> Being able to pick aparticular timestamp is an oddly specific
>> application; my impression of general SenML data sets is that it will be
>> hard to predict which point in time should have a datum if that record
>> is not already known. Unless there are good (and argued for) use cases,
>> I suggest that the topic of fetching time series points be explored
>> comprehensively in a separate document, possibly hooking into
>> draft-bormann-t2trg-stp.
> 
> The time-stamp use case I had in mind was where the record is already known, so it's mainly useful for PATCHing, but didn't see issue having it for FETCH too (say, you know time out of band). Otherwise we have no mechanism to tell apart two records from the same source.
> 
> 
>> Using SenML to fetch
>> --------------------
>>
>> SenML requires to have a value (or sum) on every record, the payload of
>> the fetch request therefore can't use any of the defined SenML mime
>> types but would need to specify their own. I'm unsure on whether adding
>> a field like "fetch_":1 to the first record (a mandatory-to-decode
>> custom field) would be sufficient to work around that constraint.
>>
>> Otherwise, it might be an option to update RFC8428 to say that some of
>> those limitations (also the later "v":null) may be overridden in
>> applications like this where it is made sure (eg. by never using those
>> freedoms in Content or PUT payloads) that no generic SenML processor
>> ever gets its hands on the document.
> 
> Good point. Maybe updating 8428 with the exception that restrictions on value don't apply for FETCH/PATCH is appropriate.
> 
>> The payload sent in as a FETCH payload is not actually sensor data, it's
>> a list of names (or, provided they're viewed as URIs, which I recommend)
>> links. We already have a way to transmit a list of links in CoRE
>> (actually 3 with links-json). I don't object to introducing another
>> (given the above, might be called application/links+senml+{cbor,json}?)
>> especially to reduce parsing complexity, but do suggest that FETCHing
>> SenML data be done with a list of identifiers no matter how they are
>> transported, and the recommended way can well be a SenML-names-only
>> payload (but would not preclude others).
> 
> The payload is a list of names without URI semantics (e.g., may not have scheme at all, have different concatenation semantics, etc.) and timestamps, so I think it's quite different from the other link formats.
> 
>> Patch
>> =====
>>
>> "value field with value null": There has been some backpressure against
>> having variable-type fields in SenML back when its JSON/CBOR structure
>> was last changed; having a "v":null in there could be problematic (esp.
>> as its JSON type is fixed to Number in Table 2 of RFC8428). I suggest
>> having a explicit "delete value" field, eg. "vdel":true.
> 
> The "v":null approach made this nice and clean, but you do have a valid point to consider here. Will have a closer look at this.
> 
>> How would base values work with patch? I figure that it's always the
>> resolved record that matter, but that should be explicit. In connection
>> with a "vdel" (or "v":null if it turns out it's OK for it to stay like
>> that), it might make sense to allow that too (where a "v":n would
>> override "vdel" or "v":null from an offset of zero) to allow for
>> efficient batch removal of values.
> 
> Yes, resolved records are that matter (see end of section 3.1). I'm not quite sure what you suggest allowing though?
> 
>> Security considerations
>> =======================
>>
>> "potentially allow retrieving or changing many resources at once" makes
>> it sound like FETCH and PATCH could reach out to different resources.
>> They don't (conceptually), they only manipulate what's inside the target
>> resource (which typically is an aggregation of other resources, as in a
>> core-interfaces batch). Suggested replacement:
>>
>>    In FETCH and iPATCH 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.
>>
>>    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.
>>
>> (It's fine if someone goes ahead and defines a /senml resource on their
>> device that is a gate to "everything and anything that can be addressed
>> in SenML", but that shouldn't be the default assumption).
> 
> Good point. The replacement looks good to me.
> 
> 
> Thanks,
> Ari
> 
>> Best regards
>> Christian
>>
>> -- 
>> You don't become great by trying to be great. You become great by
>> wanting to do something, and then doing it so hard that you become great
>> in the process.
>>   -- Marie Curie (as quoted by Randall Munroe)
> 
>